| Message ID | 20220323140215.2568-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/4] spidev: Do not use atomic bit operations when allocating minor | expand |
On Wed, Mar 23, 2022 at 04:02:12PM +0200, Andy Shevchenko wrote: > There is no need to use atomic bit operations when allocating a minor > number since it's done under a mutex. Moreover, one of the operations > that is in use is non-atomic anyway. > if (status == 0) { > - set_bit(minor, minors); > + __set_bit(minor, minors); > list_add(&spidev->device_entry, &device_list); There's no *need* but the __ looks suspicious... what's the upside here?
On Wed, Mar 23, 2022 at 04:39:01PM +0000, Mark Brown wrote: > On Wed, Mar 23, 2022 at 04:02:12PM +0200, Andy Shevchenko wrote: > > There is no need to use atomic bit operations when allocating a minor > > number since it's done under a mutex. Moreover, one of the operations > > that is in use is non-atomic anyway. ... > > if (status == 0) { > > - set_bit(minor, minors); > > + __set_bit(minor, minors); > > list_add(&spidev->device_entry, &device_list); > > There's no *need* but the __ looks suspicious... what's the upside > here? It's exactly what is written in the commit message __*_bit() are non-atomic *_bit() are atomic Since they are wrapped by mutex, the atomic ones are not needed.
On Wed, Mar 23, 2022 at 07:22:52PM +0200, Andy Shevchenko wrote: > On Wed, Mar 23, 2022 at 04:39:01PM +0000, Mark Brown wrote: > > There's no *need* but the __ looks suspicious... what's the upside > > here? > It's exactly what is written in the commit message > __*_bit() are non-atomic > *_bit() are atomic > Since they are wrapped by mutex, the atomic ones are not needed. Yes, it's not needed but what meaningful harm does it do?
On Wed, Mar 23, 2022 at 07:06:25PM +0000, Mark Brown wrote: > On Wed, Mar 23, 2022 at 07:22:52PM +0200, Andy Shevchenko wrote: > > On Wed, Mar 23, 2022 at 04:39:01PM +0000, Mark Brown wrote: > > > > There's no *need* but the __ looks suspicious... what's the upside > > > here? > > > It's exactly what is written in the commit message > > > __*_bit() are non-atomic > > *_bit() are atomic > > > Since they are wrapped by mutex, the atomic ones are not needed. > > 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.
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.
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?
On Wed, Mar 23, 2022 at 04:02:12PM +0200, Andy Shevchenko wrote: > There is no need to use atomic bit operations when allocating a minor > number since it's done under a mutex. Moreover, one of the operations > that is in use is non-atomic anyway. Shall I resend the series without this patch, or can you apply the rest if you have no comments / objections?
On Fri, Mar 25, 2022 at 02:44:34PM +0200, Andy Shevchenko wrote: > On Wed, Mar 23, 2022 at 04:02:12PM +0200, Andy Shevchenko wrote: > > There is no need to use atomic bit operations when allocating a minor > > number since it's done under a mutex. Moreover, one of the operations > > that is in use is non-atomic anyway. > Shall I resend the series without this patch, or can you apply the rest > if you have no comments / objections? I've already queued the rest.
On Fri, Mar 25, 2022 at 01:26:51PM +0000, Mark Brown wrote: > On Fri, Mar 25, 2022 at 02:44:34PM +0200, Andy Shevchenko wrote: > > On Wed, Mar 23, 2022 at 04:02:12PM +0200, Andy Shevchenko wrote: ... > > Shall I resend the series without this patch, or can you apply the rest > > if you have no comments / objections? > > I've already queued the rest. Thanks!
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
On Wed, 23 Mar 2022 16:02:12 +0200, Andy Shevchenko wrote: > There is no need to use atomic bit operations when allocating a minor > number since it's done under a mutex. Moreover, one of the operations > that is in use is non-atomic anyway. > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [2/4] spidev: Convert BUILD_BUG_ON() to static_assert() commit: d21b94bf3ac44aa7759c0de6f72c0a887eb9e23b [3/4] spidev: Replace ACPI specific code by device_get_match_data() commit: 2a7f669dd8f6561d227e724ca2614c25732f4799 [4/4] spidev: Replace OF specific code by device property API commit: 88a285192084edab6657e819f7f130f9cfcb0579 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index 53a551714265..daeaa4a30290 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -795,7 +795,7 @@ static int spidev_probe(struct spi_device *spi) status = -ENODEV; } if (status == 0) { - set_bit(minor, minors); + __set_bit(minor, minors); list_add(&spidev->device_entry, &device_list); } mutex_unlock(&device_list_lock); @@ -823,7 +823,7 @@ static void spidev_remove(struct spi_device *spi) list_del(&spidev->device_entry); device_destroy(spidev_class, spidev->devt); - clear_bit(MINOR(spidev->devt), minors); + __clear_bit(MINOR(spidev->devt), minors); if (spidev->users == 0) kfree(spidev); mutex_unlock(&device_list_lock);
There is no need to use atomic bit operations when allocating a minor number since it's done under a mutex. Moreover, one of the operations that is in use is non-atomic anyway. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/spi/spidev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)