Re: [virtio-dev] [PATCH v9] virtio-net: add Max MTU configuration field

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Sep 07, 2016 at 11:18:22AM +0200, Cornelia Huck wrote:
> On Tue,  6 Sep 2016 10:31:01 -0400
> Aaron Conole <aconole@redhat.com> wrote:
> 
> <sorry for being late to the party, but I just got around to looking at
> this as this is up for vote>
> 
> > diff --git a/content.tex b/content.tex
> > index 4b45678..b90cbad 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -3049,6 +3049,14 @@ features.
> >  \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
> >          reconfiguration support.
> > 
> > +\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported. If
> > +    offered by the device, device advises driver about the value of
> > +    MTU to be used. If negotiated, the driver uses \field{mtu} as
> > +    the maximum MTU value supplied to the operating system.
> > +
> > +    Note: many operating systems override the MTU value provided by the
> > +    driver.
> 
> I'm wondering: Do we need to distinguish between what the _driver_ does
> and what the _guest_ does, generally speaking?
> 
> > +
> >  \item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
> > 
> >  \item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
> > @@ -3140,11 +3148,16 @@ of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
> >  and transmitq1\ldots transmitqN respectively) that can be configured once VIRTIO_NET_F_MQ
> >  is negotiated.
> > 
> > +The following driver-read-only field, \field{mtu} only exists if
> > +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
> > +use.
> > +
> >  \begin{lstlisting}
> >  struct virtio_net_config {
> >          u8 mac[6];
> >          le16 status;
> >          le16 max_virtqueue_pairs;
> > +        le16 mtu;
> >  };
> >  \end{lstlisting}
> > 
> > @@ -3153,6 +3166,18 @@ struct virtio_net_config {
> >  The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 inclusive,
> >  if it offers VIRTIO_NET_F_MQ.
> > 
> > +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
> > +if it offers VIRTIO_NET_F_MTU.
> > +
> > +The device MUST NOT modify \field{mtu} once it has been set.
> 
> Should this rather be relaxed to "after VIRTIO_NET_F_MTU has been
> successfully negotiated"? I don't think the driver should assume
> anything until after feature negotiation is complete.

I don't think this matters much, but generally it's best
if the config space can be read at any time.
For example, this can allow validating MTU and not negotiating
it if it's e.g. too small.
Do you see value in tweaking the MTU after negotiation?


> > +
> > +The device MUST NOT pass received packets that exceed \field{mtu} size
> > +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.
> 
> I don't think it should be the offer that is the deciding factor, but
> rather the final negotiation.

Makes sense.

> But maybe we can make this "SHOULD NOT"?

It's important to make this a MUST because otherwise device
needs to do something if it gets a bigger packet -
e.g. fragment it, or drop it.
With this wording, it does not have to.


> > +
> > +The device MUST forward transmitted packets of up to MTU size with
> > +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
> > +offers VIRTIO_NET_F_MTU.
> > +
> >  \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
> > 
> >  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> > @@ -3165,6 +3190,15 @@ If the driver does not negotiate the VIRTIO_NET_F_STATUS feature, it SHOULD
> >  assume the link is active, otherwise it SHOULD read the link status from
> >  the bottom bit of \field{status}.
> > 
> > +If the device offers VIRTIO_NET_F_MTU, a driver MUST supply enough receive
> > +buffers of size \field{mtu} to be able to receive at least one receive
> > +packet with \field{gso_type} NONE or ECN.
> 
> Again, this should be the final negotiation instead of the offer,
> especially as...

Agree here - we also can't declare all existing drivers nonconforming.


> > +
> > +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
> 
> ...this is SHOULD and not MUST. The driver should be able to fail
> negotiating the feature if it cannot fulfill the condition above.

For that to work, MTU must not change before negotiation though.

> > +
> > +If the device offers VIRTIO_NET_F_MTU, a driver MUST NOT transmit packets of
> > +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
> > +
> >  \subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout / Legacy Interface: Device configuration layout}
> >  \label{sec:Device Types / Block Device / Feature bits / Device configuration layout / Legacy Interface: Device configuration layout}
> >  When using the legacy interface, transitional devices and drivers
> 
> Again, sorry for being late with comments, but I'll vote with no for
> now. I can still change the vote if you can convince me, though.

Aaron, what's your take? Should I withdraw the ballot for now?