netdev - Re: [PATCH net] vmxnet3: use gro callback when UPT is enabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 15 Mar 2023 09:51:47 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     Ronak Doshi <doshir@...are.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Pv-drivers <Pv-drivers@...are.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Guolin Yang <gyang@...are.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] vmxnet3: use gro callback when UPT is enabled

On 2023/3/15 5:09, Ronak Doshi wrote:
> 
> > On 3/9/23, 5:02 PM, "Yunsheng Lin" <linyunsheng@...wei.com <mailto:linyunsheng@...wei.com>> wrote:
>>
>> So it is a run time thing? What happens if some LRO'ed packet is put in the rx queue,
>> and the the vnic switches the mode to UPT, is it ok for those LRO'ed packets to go through
>> the software GSO processing?
> Yes, it should be fine.
> 
>> If yes, why not just call napi_gro_receive() for LRO case too?
>>
> We had done perf measurements in the past and it turned out this results in perf penalty.
> See https://patchwork.ozlabs.org/project/netdev/patch/1308947605-4300-1-git-send-email-jesse@nicira.com/
> 
> In fact, internally recently we did some perf measurements on RHEL 9.0, and it still showed some penalty.

Does clearing the NETIF_F_GRO for netdev->features bring back the performance?
If no, maybe there is something need investigating.

> 
>> Looking closer, it seems vnic is implementing hw GRO from driver' view, as the driver is
>> setting skb_shinfo(skb)->gso_* accordingly:
>>
>>
>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fnet%2Fvmxnet3%2Fvmxnet3_drv.c%23L1665&data=05%7C01%7Cdoshir%40vmware.com%7C68e4b3dbd7d948887f0808db21031e2c%>7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C638140069565449054%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=LAw6oCG2MgYH4TPQAnWUy25E2u%2FDMSW2aSJ7OY2%2FOu8%3D&reserved=0 <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fnet%2Fvmxnet3%2Fvmxnet3_drv.c%23L1665&amp;data=05%7C01%7Cdoshir%40vmware.com%7C68e4b3dbd7d948887f0808db21031e2c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C638140069565449054%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=LAw6oCG2MgYH4TPQAnWUy25E2u%2FDMSW2aSJ7OY2%2FOu8%3D&amp;reserved=0>
>>
>>
>> In that case, you may call napi_gro_receive() for those GRO'ed skb too, see:
>>
> 
> I see. Seems this got added recently. This will need re-evaluation by the team based on ToT Linux.
> But this can be done in near future and as this might take time, for now this patch should be applied as
> UPT patches are already up-streamed.

Checking rq->shared->updateRxProd in the driver to decide if gro is allow does not seems right to
me, as the netstack has used the NETIF_F_GRO checking in netif_elide_gro().

Does clearing NETIF_F_GRO for netdev->features during the driver init process works for your
case?

As netdev->hw_features is for the driver to advertise the hw's capability, and the driver
can enable/disable specific capability by setting netdev->features during the driver init
process, and user can get to enable/disable specific capability using ethtool later if user
need to.

> 
> Thanks, 
> Ronak 
> 
>