Messages in this thread | | | Date | Fri, 25 Mar 2022 18:29:45 -0700 | From | Yury Norov <> | Subject | Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor |
| |
On Fri, Mar 25, 2022 at 01:09:36PM +0200, Andy Shevchenko wrote: > On Thu, Mar 24, 2022 at 10:48 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Mar 24, 2022 at 11:24:26AM +0200, Andy Shevchenko wrote: > > > On Wed, Mar 23, 2022 at 07:06:25PM +0000, Mark Brown wrote: > > > > > > Yes, it's not needed but what meaningful harm does it do? > > > > > There are basically two points: > > > > > 1) in one driver the additional lock may not be influential, but > > > if many drivers will do the same, it will block CPUs for no > > > purpose; > > > > > 2) derived from the above, if one copies'n'pastes the code, esp. > > > using spin locks, it may become an unneeded code and performance > > > degradation. > > > > I think if these are serious issues they need to be addressed in the API > > so that code doing the fancy unlocked stuff that needs atomicity is the > > code that has the __ and looks like it's doing something tricky and > > peering into internals. > > I believe the issue you mainly pointed out is the __ in the name of > the APIs, since it's case by case when you need one or the other. In > case of spidev we need non-atomic versions, in case of, e.g., > drivers/dma/dw/core.c we need atomic, because spin locks used there do > not (and IIRC may not) cover some cases where the bit operations are > used against same bitmap. > > Perhaps we might add the aliases as clear_bit_nonatomic() et al. Yury, > what do you think?
We already have bitmap_clear(addr, nr, 1), which would call __clear_bit() without any overhead. So, I think we'd encourage people to switch to bitmap_{set,clear} where it makes sense, i.e. where the object is a real bitmap, not a thing like flags. We can even invent bitmap_set_bit(addr, nr) if needed.
What really concerns me is that our atomic bit operations are not really atomic. If bitmap doesn't fit into a single word, different threads may read/write different parts of such bitmap concurrently.
Another thing is that we have no atomic versions for functions like bitmap_empty(), which means that atomic part of bitmap API cannot be mixed with non-atomic part.
This means that atomic ops are most probably used wider than it worth. I don't know how many useless atomic bitmap ops we have, I also don't know how to estimate the excessive pressure on cache subsystem generated by this useless clear/set_bit() traffic.
Thanks, Yury
| |