lkml.org 
[lkml]   [2022]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2022-03-26 02:30    [W:0.092 / U:0.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site