[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240326073722.637e8504@kernel.org>
Date: Tue, 26 Mar 2024 07:37:22 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Johannes Berg <johannes@...solutions.net>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH 0/3] using guard/__free in networking
On Tue, 26 Mar 2024 09:42:43 +0100 Johannes Berg wrote:
> On Mon, 2024-03-25 at 19:09 -0700, Jakub Kicinski wrote:
> > On Mon, 25 Mar 2024 23:31:25 +0100 Johannes Berg wrote:
> > > Hi,
> > >
> > > So I started playing with this for wifi, and overall that
> > > does look pretty nice, but it's a bit weird if we can do
> > >
> > > guard(wiphy)(&rdev->wiphy);
> > >
> > > or so, but still have to manually handle the RTNL in the
> > > same code.
> >
> > Dunno, it locks code instead of data accesses.
>
> Well, I'm not sure that's a fair complaint. After all, without any more
> compiler help, even rtnl_lock()/rtnl_unlock() _necessarily_ locks code.
> Clearly
>
> rtnl_lock();
> // something
> rtnl_unlock();
>
> also locks the "// something" code, after all., and yeah that might be
> doing data accesses, but it might also be a function call or a whole
> bunch of other things?
>
> Or if you look at something like bpf_xdp_link_attach(), I don't think
> you can really say that it locks only data. That doesn't even do the
> allocation outside the lock (though I did convert that one to
> scoped_guard because of that.)
>
> Or even something simple like unregister_netdev(), it just requires the
> RTNL for some data accesses and consistency deep inside
> unregister_netdevice(), not for any specific data accessed there.
>
> So yeah, this is always going to be a trade-off, but all the locking is.
> We even make similar trade-offs manually, e.g. look at
> bpf_xdp_link_update(), it will do the bpf_prog_put() under the RTNL
> still, for no good reason other than simplifying the cleanup path there.
At least to me the mental model is different. 99% of the time the guard
is covering the entire body. So now we're moving from "I'm touching X
so I need to lock" to "This _function_ is safe to touch X".
> Anyway, I can live with it either way (unless you tell me you won't pull
> wireless code using guard), just thought doing the wireless locking with
> guard and the RTNL around it without it (only in a few places do we
> still use RTNL though) looked odd.
>
>
> > Forgive the comparison but it feels too much like Java to me :)
>
> Heh. Haven't used Java in 20 years or so...
I only did at uni, but I think they had a decorator for a method, where
you can basically say "this method should be under lock X" and runtime
will take that lock before entering and drop it after exit,
appropriately. I wonder why the sudden love for this concept :S
Is it also present in Rust or some such?
> > scoped_guard is fine, the guard() not so much.
>
> I think you can't get scoped_guard() without guard(), so does that mean
> you'd accept the first patch in the series?
How can we get one without the other.. do you reckon Joe P would let us
add a checkpatch check to warn people against pure guard() under net/ ?
> > Do you have a piece of code in wireless where the conversion
> > made you go "wow, this is so much cleaner"?
>
> Mostly long and complex error paths. Found a double-unlock bug (in
> iwlwifi) too, when converting some locking there.
>
> Doing a more broader conversion on cfg80211/mac80211 removes around 200
> lines of unlocking, mostly error handling, code.
>
> Doing __free() too will probably clean up even more.
Not super convinced by that one either:
https://lore.kernel.org/all/20240321185640.6f7f4d6b@kernel.org/
maybe I'm too conservative..
Powered by blists - more mailing lists