diff mbox series

[v1,1/4] spidev: Do not use atomic bit operations when allocating minor

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

Commit Message

Andy Shevchenko March 23, 2022, 2:02 p.m. UTC
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(-)

Comments

Mark Brown March 23, 2022, 4:39 p.m. UTC | #1
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?
Andy Shevchenko March 23, 2022, 5:22 p.m. UTC | #2
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.
Mark Brown March 23, 2022, 7:06 p.m. UTC | #3
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?
Andy Shevchenko March 24, 2022, 9:24 a.m. UTC | #4
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.
Mark Brown March 24, 2022, 2:28 p.m. UTC | #5
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.
Andy Shevchenko March 25, 2022, 11:09 a.m. UTC | #6
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?
Andy Shevchenko March 25, 2022, 12:44 p.m. UTC | #7
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?
Mark Brown March 25, 2022, 1:26 p.m. UTC | #8
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.
Andy Shevchenko March 25, 2022, 1:44 p.m. UTC | #9
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!
Yury Norov March 26, 2022, 1:29 a.m. UTC | #10
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
Mark Brown April 5, 2022, 9:32 a.m. UTC | #11
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 mbox series

Patch

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);