linux-kernel - Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yj5siWiBVKUgtbxz@yury-laptop>
Date:   Fri, 25 Mar 2022 18:29:45 -0700
From:   Yury Norov <yury.norov@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Mark Brown <broonie@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-spi <linux-spi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
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@...nel.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