Re: [PATCH net v2 1/3] macsec: introduce dedicated workqueue for SA crypto cleanup

[Jakub Kicinski <kuba@kernel.org>]

On Sat,  9 May 2026 10:00:54 +0800 alexjlzheng@gmail.com wrote:
> From: Jinliang Zheng <alexjlzheng@tencent.com>
> 
> On Fri, 8 May 2026 15:42:43 -0700, Jakub Kicinski wrote:
> > > +wq:
> > > +	destroy_workqueue(macsec_wq);
> > > +	return err;  
> >
> > nit: err_destroy_wq: would be a better label name
> >
> > rcu_barrier() is needed before this  
> 
> Thanks for the review.
> 
> Regarding the label name: "wq:" follows the existing naming convention
> in macsec_init(), where the other error labels are also named after the
> resource they clean up ("rtnl:", "notifier:") rather than using the
> "err_xxx:" style. I kept it consistent with that local convention, but
> happy to switch to "err_destroy_wq:" if you prefer uniformity with the
> broader codebase.

I can see that, but we have to start somewhere.

> Regarding rcu_barrier(): in the error path of macsec_init(), the
> workqueue has just been created and no MACsec interface has been
> registered yet.  queue_rcu_work(macsec_wq, ...) is only reachable
> from macsec_rxsa_put() and macsec_txsa_put(), which require an SA
> object to exist. No SA can be created before macsec_init() succeeds,
> so macsec_wq is guaranteed to be empty at this point and
> rcu_barrier() is not needed here.
> 
> The rcu_barrier() in macsec_exit() is necessary for a different
> reason: at that point live interfaces may have been destroyed and
> their SAs may be in-flight through the rcu_work mechanism, so we
> must wait for all rcu_work_rcufn callbacks to finish enqueuing their
> work items before destroy_workqueue() drains them.
> 
> Does that reasoning make sense, or am I missing a path where work
> could be queued onto macsec_wq during the init error unwind?

TBH I didn't check the code, you may be right that until the genl
family is registered there's no way to an SA to exist (even tho
macsec devices may have already gotten created).

Either way, I'd just add that barrier. We're basically leaving
a trap for whoever tries to add another piece of code to this function.
The error unwind path and the remove path should generally be kept
the same.