2013-03-08 15:07:31

by Maarten Lankhorst

Subject: [PATCH] sysfs: use atomic_inc_unless_negative in sysfs_get_active

It seems that sysfs has an interesting way of doing the same thing.
This removes the cpu_relax unfortunately, but if it's really needed,
it would be better to add this to include/linux/atomic.h to benefit
all atomic ops users.

Signed-off-by: Maarten Lankhorst <[email protected]>
---

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 2fbdff6..7f968ed 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -165,21 +165,8 @@ struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
if (unlikely(!sd))
return NULL;

- while (1) {
- int v, t;
-
- v = atomic_read(&sd->s_active);
- if (unlikely(v < 0))
- return NULL;
-
- t = atomic_cmpxchg(&sd->s_active, v, v + 1);
- if (likely(t == v))
- break;
- if (t < 0)
- return NULL;
-
- cpu_relax();
- }
+ if (!atomic_inc_unless_negative(&sd->s_active))
+ return NULL;

if (likely(!ignore_lockdep(sd)))
rwsem_acquire_read(&sd->dep_map, 0, 1, _RET_IP_);

2013-03-19 00:13:04

by Greg Kroah-Hartman

Subject: Re: [PATCH] sysfs: use atomic_inc_unless_negative in sysfs_get_active

On Fri, Mar 08, 2013 at 04:07:27PM +0100, Maarten Lankhorst wrote:
> It seems that sysfs has an interesting way of doing the same thing.
> This removes the cpu_relax unfortunately, but if it's really needed,
> it would be better to add this to include/linux/atomic.h to benefit
> all atomic ops users.
>
> Signed-off-by: Maarten Lankhorst <[email protected]>
> ---

Tejun wrote this code originally, so I'll defer to him as to if the
patch below is a valid cleanup or not.

Tejun?


>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 2fbdff6..7f968ed 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -165,21 +165,8 @@ struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
> if (unlikely(!sd))
> return NULL;
>
> - while (1) {
> - int v, t;
> -
> - v = atomic_read(&sd->s_active);
> - if (unlikely(v < 0))
> - return NULL;
> -
> - t = atomic_cmpxchg(&sd->s_active, v, v + 1);
> - if (likely(t == v))
> - break;
> - if (t < 0)
> - return NULL;
> -
> - cpu_relax();
> - }
> + if (!atomic_inc_unless_negative(&sd->s_active))
> + return NULL;
>
> if (likely(!ignore_lockdep(sd)))
> rwsem_acquire_read(&sd->dep_map, 0, 1, _RET_IP_);

2013-03-19 00:29:49

by Tejun Heo

Subject: Re: [PATCH] sysfs: use atomic_inc_unless_negative in sysfs_get_active

On Mon, Mar 18, 2013 at 05:14:18PM -0700, Greg KH wrote:
> On Fri, Mar 08, 2013 at 04:07:27PM +0100, Maarten Lankhorst wrote:
> > It seems that sysfs has an interesting way of doing the same thing.
> > This removes the cpu_relax unfortunately, but if it's really needed,
> > it would be better to add this to include/linux/atomic.h to benefit
> > all atomic ops users.
> >
> > Signed-off-by: Maarten Lankhorst <[email protected]>
> > ---
>
> Tejun wrote this code originally, so I'll defer to him as to if the
> patch below is a valid cleanup or not.

Yeah, I think that code predates atomic_inc_unless_negative().

> > diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> > index 2fbdff6..7f968ed 100644
> > --- a/fs/sysfs/dir.c
> > +++ b/fs/sysfs/dir.c
> > @@ -165,21 +165,8 @@ struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
> > if (unlikely(!sd))
> > return NULL;
> >
> > - while (1) {
> > - int v, t;
> > -
> > - v = atomic_read(&sd->s_active);
> > - if (unlikely(v < 0))
> > - return NULL;
> > -
> > - t = atomic_cmpxchg(&sd->s_active, v, v + 1);
> > - if (likely(t == v))
> > - break;
> > - if (t < 0)
> > - return NULL;
> > -
> > - cpu_relax();
> > - }
> > + if (!atomic_inc_unless_negative(&sd->s_active))
> > + return NULL;

Looks good to me.

Acked-by: Tejun Heo <[email protected]>

Thanks.

--
tejun