Virtualization - Aug 2022 - [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

> From: Michael S. Tsirkin <mst at redhat.com>
> Sent: Tuesday, August 9, 2022 6:26 PM
> To: Parav Pandit <parav at nvidia.com>
> Cc: Si-Wei Liu <si-wei.liu at oracle.com>; Jason Wang
> <jasowang at redhat.com>; Gavin Li <gavinl at nvidia.com>;
Hemminger,
> Stephen <stephen at networkplumber.org>; davem
> <davem at davemloft.net>; virtualization <virtualization at
lists.linux-
> foundation.org>; Virtio-Dev <virtio-dev at lists.oasis-open.org>;
> jesse.brandeburg at intel.com; alexander.h.duyck at intel.com;
> kubakici at wp.pl; sridhar.samudrala at intel.com; loseweigh at gmail.com;
Gavi
> Teitz <gavi at nvidia.com>
> Subject: Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length
for
> big packets
> 
> On Tue, Aug 09, 2022 at 09:49:03PM +0000, Parav Pandit wrote:
> > > From: Michael S. Tsirkin <mst at redhat.com>
> > > Sent: Tuesday, August 9, 2022 5:38 PM
> >
> > [..]
> > > > > I think virtio-net driver doesn't differentiate MTU
and MRU, in
> > > > > which case the receive buffer will be reduced to fit
the 1500B
> > > > > payload size when mtu is lowered down to 1500 from
9000.
> > > > How? Driver reduced the mXu to 1500, say it is improved to
post
> > > > buffers of
> > > 1500 bytes.
> > > >
> > > > Device doesn't know about it because mtu in config space
is RO field.
> > > > Device keep dropping 9K packets because buffers posted are
1500
> bytes.
> > > > This is because device follows the spec " The device
MUST NOT pass
> > > received packets that exceed mtu".
> > >
> > >
> > > The "mtu" here is the device config field, which is
> > >
> > >         /* Default maximum transmit unit advice */
> > >
> >
> > It is the field from struct virtio_net_config.mtu. right?
> > This is RO field for driver.
> >
> > > there is no guarantee device will not get a bigger packet.
> > Right. That is what I also hinted.
> > Hence, allocating buffers worth upto mtu is safer.
> 
> yes
> 
> > When user overrides it, driver can be further optimized to honor such
new
> value on rx buffer posting.
> 
> no, not without a feature bit promising device won't get wedged.
> 
I mean to say as_it_stands today, driver can decide to post smaller buffers with
larger mtu.
Why device should be affected with it?
( I am not proposing such weird configuration but asking for sake of
correctness).
> > > And there is no guarantee such a packet will be dropped as
opposed
> > > to wedging the device if userspace insists on adding smaller
buffers.
> > >
> > If user space insists on small buffers, so be it.
> 
> If previously things worked, the "so be it" is a regression and
blaming users
> won't help us.
> 
I am not suggesting above.
This was Si-Wei's suggestion that somehow driver wants to post smaller
buffers than the mtu because user knows what peer is doing.
So may be driver can be extended to give more weight on user config.
> > It only works when user exactly know what user is doing in the whole
> network.
> 
> If you want to claim this you need a new feature bit.
> 
Why is a new bit needed to tell device?
User is doing something its own config mismatching the buffers and mtu.
A solid use case hasn't emerged for this yet.

If user wants to modify the mtu, we should just make virtio_net_config.mtu as RW
field using new feature bit.
Is that what you mean?
If so, yes, it makes things very neat where driver and device are aligned to
each other, the way they are today.
Only limitation is that its one-way. = device tells to driver.
> > When user prefers to override the device RO field, device is in the
dark and
> things work on best effort basis.
> 
> Dropping packets is best effort. Getting stuck forever isn't,
that's a quality of
> implementation issue.
>
Not sure, why things get stuck for ever. Maybe you have example to explain.
I am mostly missing something.
 
> > This must be a reasonably advance user who has good knowledge of its
> network topology etc.
> >
> > For such case, may be yes, driver should be further optimized.
> >

On Tue, Aug 09, 2022 at 10:49:48PM +0000, Parav Pandit
wrote:
> > From: Michael S. Tsirkin <mst at redhat.com>
> > Sent: Tuesday, August 9, 2022 6:26 PM
> > To: Parav Pandit <parav at nvidia.com>
> > Cc: Si-Wei Liu <si-wei.liu at oracle.com>; Jason Wang
> > <jasowang at redhat.com>; Gavin Li <gavinl at nvidia.com>;
Hemminger,
> > Stephen <stephen at networkplumber.org>; davem
> > <davem at davemloft.net>; virtualization <virtualization at
lists.linux-
> > foundation.org>; Virtio-Dev <virtio-dev at
lists.oasis-open.org>;
> > jesse.brandeburg at intel.com; alexander.h.duyck at intel.com;
> > kubakici at wp.pl; sridhar.samudrala at intel.com; loseweigh at
gmail.com; Gavi
> > Teitz <gavi at nvidia.com>
> > Subject: Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer
length for
> > big packets
> > 
> > On Tue, Aug 09, 2022 at 09:49:03PM +0000, Parav Pandit wrote:
> > > > From: Michael S. Tsirkin <mst at redhat.com>
> > > > Sent: Tuesday, August 9, 2022 5:38 PM
> > >
> > > [..]
> > > > > > I think virtio-net driver doesn't
differentiate MTU and MRU, in
> > > > > > which case the receive buffer will be reduced to
fit the 1500B
> > > > > > payload size when mtu is lowered down to 1500 from
9000.
> > > > > How? Driver reduced the mXu to 1500, say it is improved
to post
> > > > > buffers of
> > > > 1500 bytes.
> > > > >
> > > > > Device doesn't know about it because mtu in config
space is RO field.
> > > > > Device keep dropping 9K packets because buffers posted
are 1500
> > bytes.
> > > > > This is because device follows the spec " The
device MUST NOT pass
> > > > received packets that exceed mtu".
> > > >
> > > >
> > > > The "mtu" here is the device config field, which
is
> > > >
> > > >         /* Default maximum transmit unit advice */
> > > >
> > >
> > > It is the field from struct virtio_net_config.mtu. right?
> > > This is RO field for driver.
> > >
> > > > there is no guarantee device will not get a bigger packet.
> > > Right. That is what I also hinted.
> > > Hence, allocating buffers worth upto mtu is safer.
> > 
> > yes
> > 
> > > When user overrides it, driver can be further optimized to honor
such new
> > value on rx buffer posting.
> > 
> > no, not without a feature bit promising device won't get wedged.
> > 
> I mean to say as_it_stands today, driver can decide to post smaller buffers
with larger mtu.
> Why device should be affected with it?
> ( I am not proposing such weird configuration but asking for sake of
correctness).
They just are because drivers did not do this.
> > > > And there is no guarantee such a packet will be dropped as
opposed
> > > > to wedging the device if userspace insists on adding smaller
buffers.
> > > >
> > > If user space insists on small buffers, so be it.
> > 
> > If previously things worked, the "so be it" is a regression
and blaming users
> > won't help us.
> > 
> I am not suggesting above.
> This was Si-Wei's suggestion that somehow driver wants to post smaller
buffers than the mtu because user knows what peer is doing.
> So may be driver can be extended to give more weight on user config.
> 
> > > It only works when user exactly know what user is doing in the
whole
> > network.
> > 
> > If you want to claim this you need a new feature bit.
> > 
> Why is a new bit needed to tell device?
> User is doing something its own config mismatching the buffers and mtu.
> A solid use case hasn't emerged for this yet.
> 
> If user wants to modify the mtu, we should just make virtio_net_config.mtu
as RW field using new feature bit.
> Is that what you mean?
> If so, yes, it makes things very neat where driver and device are aligned
to each other, the way they are today.
> Only limitation is that its one-way. = device tells to driver.
> 
> > > When user prefers to override the device RO field, device is in
the dark and
> > things work on best effort basis.
> > 
> > Dropping packets is best effort. Getting stuck forever isn't,
that's a quality of
> > implementation issue.
> >
> Not sure, why things get stuck for ever. Maybe you have example to explain.
> I am mostly missing something.
I sent an explanation a bit earlier. It's more or less a bug.
> > > This must be a reasonably advance user who has good knowledge of
its
> > network topology etc.
> > >
> > > For such case, may be yes, driver should be further optimized.
> > >

On Tue, Aug 09, 2022 at 10:49:48PM +0000, Parav Pandit
wrote:
> > > When user prefers to override the device RO field, device is in
the dark and
> > things work on best effort basis.
> > 
> > Dropping packets is best effort. Getting stuck forever isn't,
that's a quality of
> > implementation issue.
> >
> Not sure, why things get stuck for ever. Maybe you have example to explain.
> I am mostly missing something.
I'm no longer sure I'm right. Will recheck tomorrow, it's late here.

-- 
MST

On 8/9/2022 3:49 PM, Parav Pandit wrote:
>> From: Michael S. Tsirkin <mst at redhat.com>
>> Sent: Tuesday, August 9, 2022 6:26 PM
>> To: Parav Pandit <parav at nvidia.com>
>> Cc: Si-Wei Liu <si-wei.liu at oracle.com>; Jason Wang
>> <jasowang at redhat.com>; Gavin Li <gavinl at nvidia.com>;
Hemminger,
>> Stephen <stephen at networkplumber.org>; davem
>> <davem at davemloft.net>; virtualization <virtualization at
lists.linux-
>> foundation.org>; Virtio-Dev <virtio-dev at
lists.oasis-open.org>;
>> jesse.brandeburg at intel.com; alexander.h.duyck at intel.com;
>> kubakici at wp.pl; sridhar.samudrala at intel.com; loseweigh at
gmail.com; Gavi
>> Teitz <gavi at nvidia.com>
>> Subject: Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer
length for
>> big packets
>>
>> On Tue, Aug 09, 2022 at 09:49:03PM +0000, Parav Pandit wrote:
>>>> From: Michael S. Tsirkin <mst at redhat.com>
>>>> Sent: Tuesday, August 9, 2022 5:38 PM
>>> [..]
>>>>>> I think virtio-net driver doesn't differentiate MTU
and MRU, in
>>>>>> which case the receive buffer will be reduced to fit
the 1500B
>>>>>> payload size when mtu is lowered down to 1500 from
9000.
>>>>> How? Driver reduced the mXu to 1500, say it is improved to
post
>>>>> buffers of
>>>> 1500 bytes.
>>>>> Device doesn't know about it because mtu in config
space is RO field.
>>>>> Device keep dropping 9K packets because buffers posted are
1500
>> bytes.
>>>>> This is because device follows the spec " The device
MUST NOT pass
>>>> received packets that exceed mtu".
>>>>
>>>>
>>>> The "mtu" here is the device config field, which is
>>>>
>>>>          /* Default maximum transmit unit advice */
>>>>
>>> It is the field from struct virtio_net_config.mtu. right?
>>> This is RO field for driver.
>>>
>>>> there is no guarantee device will not get a bigger packet.
>>> Right. That is what I also hinted.
>>> Hence, allocating buffers worth upto mtu is safer.
>> yes
>>
>>> When user overrides it, driver can be further optimized to honor
such new
>> value on rx buffer posting.
>>
>> no, not without a feature bit promising device won't get wedged.
>>
> I mean to say as_it_stands today, driver can decide to post smaller buffers
with larger mtu.
> Why device should be affected with it?
> ( I am not proposing such weird configuration but asking for sake of
correctness).
I am also confused how the device can be wedged in this case.
>
>>>> And there is no guarantee such a packet will be dropped as
opposed
>>>> to wedging the device if userspace insists on adding smaller
buffers.
>>>>
>>> If user space insists on small buffers, so be it.
>> If previously things worked, the "so be it" is a regression
and blaming users
>> won't help us.
>>
> I am not suggesting above.
> This was Si-Wei's suggestion that somehow driver wants to post smaller
buffers than the mtu because user knows what peer is doing.
> So may be driver can be extended to give more weight on user config.
It's not me, it's from our customers with real use cases. Some of which 
have very dedicate network setup, and it's not at odd they know virtio 
internals quite well. At one point they even customized the driver to 
disable mergeable buffer ahead of us offering them the opt-out from 
device level. And their appliance indeed has assumption of 1460 mtu 
everywhere.

-Siwei
>
>>> It only works when user exactly know what user is doing in the
whole
>> network.
>>
>> If you want to claim this you need a new feature bit.
>>
> Why is a new bit needed to tell device?
> User is doing something its own config mismatching the buffers and mtu.
> A solid use case hasn't emerged for this yet.
>
> If user wants to modify the mtu, we should just make virtio_net_config.mtu
as RW field using new feature bit.
> Is that what you mean?
> If so, yes, it makes things very neat where driver and device are aligned
to each other, the way they are today.
> Only limitation is that its one-way. = device tells to driver.
>
>>> When user prefers to override the device RO field, device is in the
dark and
>> things work on best effort basis.
>>
>> Dropping packets is best effort. Getting stuck forever isn't,
that's a quality of
>> implementation issue.
>>
> Not sure, why things get stuck for ever. Maybe you have example to explain.
> I am mostly missing something.
>   
>>> This must be a reasonably advance user who has good knowledge of
its
>> network topology etc.
>>> For such case, may be yes, driver should be further optimized.
>>>