On Mon, 14 Dec 2020 09:59:21 -0800 Wei Wang wrote:

On Sat, Dec 12, 2020 at 2:55 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Sat, 12 Dec 2020 14:50:22 -0800 Jakub Kicinski wrote:  

quoted

quoted

@@ -6731,6 +6790,7 @@ void napi_disable(struct napi_struct *n)
            msleep(1);

    hrtimer_cancel(&n->timer);
+   napi_kthread_stop(n);  

I'm surprised that we stop the thread on napi_disable() but there is no
start/create in napi_enable(). NAPIs can (and do get) disabled and
enabled again. But that'd make your code crash with many popular
drivers if you tried to change rings with threaded napi enabled so I
feel like I must be missing something..  

Ah, not crash, 'cause the flag gets cleared. Is it intentional that any
changes that disable NAPIs cause us to go back to non-threaded NAPI?
I think I had the "threaded" setting stored in struct netdevice in my
patches, is there a reason not to do that?

Thanks for the comments!

The reason that I did not record it in dev is: there is a slight
chance that during creation of the kthreads, failures occur and we
flip back all NAPIs to use non-threaded mode. I am not sure the
recorded value in dev should be what user desires, or what the actual
situation is. Same as after the driver does a
napi_disabe()/napi_enable(). It might occur that the dev->threaded =
true, but the operation to re-create the kthreads fail and we flip
back to non-thread mode. This seems to get things more complicated.
What I expect is the user only enables the threaded mode after the
device is up and alive, with all NAPIs attached to dev, and enabled.
And user has to check the sysfs to make sure that the operation
succeeds.
And any operation that brings down the device, will flip this back to
default, which is non-threaded mode.

It is quite an annoying problem to address, given all relevant NAPI
helpers seem to return void :/ But we're pushing the problem onto the
user just because of internal API structure.

This reminds me of PTP / timestamping issues some NICs had once upon 
a time. The timing application enables HW time stamping, then later some
other application / orchestration changes a seemingly unrelated config,
and since NIC has to reset itself it looses the timestamping config.
Now the time app stops getting HW time stamps, but those are best
effort anyway, so it just assumes the NIC couldn't stamp given frame
(for every frame), not that config got completely broken. The system
keeps running with suboptimal time for months.

What does the deployment you're expecting to see looks like? What
entity controls enabling the threaded mode on a system? Application?
Orchestration? What's the flow?

"Forgetting" config based on driver-dependent events feels very fragile.