netdev - Re: [PATCH net] ionic: centralize queue reset code

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:   Tue, 7 Jul 2020 09:10:38 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH net] ionic: centralize queue reset code

On 7/6/20 10:33 AM, Jakub Kicinski wrote:
> On Thu,  2 Jul 2020 16:39:17 -0700 Shannon Nelson wrote:
>> The queue reset pattern is used in a couple different places,
>> only slightly different from each other, and could cause
>> issues if one gets changed and the other didn't.  This puts
>> them together so that only one version is needed, yet each
>> can have slighty different effects by passing in a pointer
>> to a work function to do whatever configuration twiddling is
>> needed in the middle of the reset.
>>
>> Fixes: 4d03e00a2140 ("ionic: Add initial ethtool support")
>> Signed-off-by: Shannon Nelson <snelson@...sando.io>
> Is this fixing anything?

Yes, this fixes issues seen similar to what was fixed with b59eabd23ee5 
("ionic: tame the watchdog timer on reconfig") where under loops of 
changing parameters we could occasionally bump into the netdev watchdog.

>
> I think the pattern of having a separate structure describing all the
> parameters and passing that into reconfig is a better path forward,
> because it's easier to take that forward in the correct direction of
> allocating new resources before old ones are freed. IOW not doing a
> full close/open.
>
> E.g. nfp_net_set_ring_size().

This has been suggested before and looks great when you know you've got 
the resources for dual allocations.  In our case this code is also used 
inside our device where memory is tight: we are much more likely to have 
allocation issues if we try to allocate everything without first 
releasing what we already have.

I agree there is room for evolution, and we have patches coming that 
change some of how we allocate our memory, but we're not quite ready to 
rewrite what we have, or to split the two driver cases yet.

sln