Parav Pandit
2022-Sep-07 19:51 UTC
[PATCH v5 2/2] virtio-net: use mtu size as buffer length for big packets
> From: Michael S. Tsirkin <mst at redhat.com> > Sent: Wednesday, September 7, 2022 3:36 PM > > On Wed, Sep 07, 2022 at 07:27:16PM +0000, Parav Pandit wrote: > > > > > From: Michael S. Tsirkin <mst at redhat.com> > > > Sent: Wednesday, September 7, 2022 3:24 PM > > > > > > On Wed, Sep 07, 2022 at 07:18:06PM +0000, Parav Pandit wrote: > > > > > > > > > From: Michael S. Tsirkin <mst at redhat.com> > > > > > Sent: Wednesday, September 7, 2022 3:12 PM > > > > > > > > > > Because of shallow queue of 16 entries deep. > > > > > > > > > > but why is the queue just 16 entries? > > > > I explained the calculation in [1] about 16 entries. > > > > > > > > [1] > > > > > > > > https://lore.kernel.org/netdev/PH0PR12MB54812EC7F4711C1EA4CAA119DC > > > 419@ > > > > PH0PR12MB5481.namprd12.prod.outlook.com/ > > > > > > > > > does the device not support indirect? > > > > > > > > > Yes, indirect feature bit is disabled on the device. > > > > > > OK that explains it. > > > > So can we proceed with v6 to contain > > (a) updated commit message and > > (b) function name change you suggested to drop _fields suffix? > > (c) replace mtu = 0 with sensibly not calling the function when mtu is > unknown.> > > And I'd like commit log to include results of perf testing > - with indirect feature onWhich device do you suggest using for this test?> - with mtu feature offWhy is this needed when it is not touching the area of mtu being not offered?> just to make sure nothing breaks.Not sure why you demand this. Can you please share the link to other patches that ensured that nothing breaks, for example I didn't see a similar "test ask" in v14 series [1]? What is so special about current patch of interest vs [1] that requires this special testing details in commit log, and it is not required in [1] or past patches? Do you have link to the tests done with synchronization tests by commit [2]? This will help to define test matrix for developers and internal regression and similar report in all subsequent patches like [1]. [1] https://lore.kernel.org/bpf/20220801063902.129329-41-xuanzhuo at linux.alibaba.com/ [2] 6213f07cb54
Si-Wei Liu
2022-Sep-07 21:39 UTC
[virtio-dev] RE: [PATCH v5 2/2] virtio-net: use mtu size as buffer length for big packets
On 9/7/2022 12:51 PM, Parav Pandit wrote:>> And I'd like commit log to include results of perf testing >> - with indirect feature on > Which device do you suggest using for this test? >You may use software vhost-net backend with and without fix to compare. Since this driver fix effectively lowers down the buffer size for the indirect=on case as well, it's a natural request to make sure no perf degradation is seen on devices with indirect descriptor enabled. I don't expect degradation though and think this patch should improve efficiency with less memory foot print. Cheers, -Siwei
Michael S. Tsirkin
2022-Sep-22 09:26 UTC
[PATCH v5 2/2] virtio-net: use mtu size as buffer length for big packets
On Wed, Sep 07, 2022 at 07:51:38PM +0000, Parav Pandit wrote:> > > From: Michael S. Tsirkin <mst at redhat.com> > > Sent: Wednesday, September 7, 2022 3:36 PM > > > > On Wed, Sep 07, 2022 at 07:27:16PM +0000, Parav Pandit wrote: > > > > > > > From: Michael S. Tsirkin <mst at redhat.com> > > > > Sent: Wednesday, September 7, 2022 3:24 PM > > > > > > > > On Wed, Sep 07, 2022 at 07:18:06PM +0000, Parav Pandit wrote: > > > > > > > > > > > From: Michael S. Tsirkin <mst at redhat.com> > > > > > > Sent: Wednesday, September 7, 2022 3:12 PM > > > > > > > > > > > > Because of shallow queue of 16 entries deep. > > > > > > > > > > > > but why is the queue just 16 entries? > > > > > I explained the calculation in [1] about 16 entries. > > > > > > > > > > [1] > > > > > > > > > > > https://lore.kernel.org/netdev/PH0PR12MB54812EC7F4711C1EA4CAA119DC > > > > 419@ > > > > > PH0PR12MB5481.namprd12.prod.outlook.com/ > > > > > > > > > > > does the device not support indirect? > > > > > > > > > > > Yes, indirect feature bit is disabled on the device. > > > > > > > > OK that explains it. > > > > > > So can we proceed with v6 to contain > > > (a) updated commit message and > > > (b) function name change you suggested to drop _fields suffix? > > > > (c) replace mtu = 0 with sensibly not calling the function when mtu is > > unknown. > > > > > > > And I'd like commit log to include results of perf testing > > - with indirect feature on > Which device do you suggest using for this test?AFAIK most devices support INDIRECT, e.g. don't nvidia cards do this?> > - with mtu feature off > Why is this needed when it is not touching the area of mtu being not offered?I don't really like it that instead of checking the MTU feature bit everywhere the patch sets mtu variable to 0. Because of this it wasn't all that obvious that the patch did not affect !MTU performance (the code does change). Rereading it afresh I think it's ok. But explicit check for !MTU would be better imho making it obvious we do not need to test !MTU. -- MST