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

[Aaron Conole <aconole@redhat.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> 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?

Sure - I will post a v10 next week with some of the critiques above
incorporated, and hopefully that version would be able to pass muster.