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
| ||
|
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
Powered by blists - more mailing lists