Date: Tue, 29 May 2007 18:13:15 +0200
From: Cornelia Huck <cornelia.huck@de.ibm.com>
To: Tejun Heo <htejun@gmail.com>
Cc: gregkh@suse.de, dmitry.torokhov@gmail.com, oneukum@suse.de,
       rpurdie@rpsys.net, stern@rowland.harvard.edu, maneesh@in.ibm.com,
       linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] sysfs: slim down sysfs_dirent->s_active
Message-ID: <20070529181315.37e31832@gondolin.boeblingen.de.ibm.com>
In-Reply-To: <11803694463144-git-send-email-htejun@gmail.com>
References: <118036944617-git-send-email-htejun@gmail.com>
	<11803694463144-git-send-email-htejun@gmail.com>
Organization: IBM Deutschland Entwicklung GmbH
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1888
Lines: 61

On Tue, 29 May 2007 01:24:06 +0900,
Tejun Heo <htejun@gmail.com> wrote:

> Make sysfs_dirent->s_active an atomic_t instead of rwsem.  This
> reduces the size of sysfs_dirent from 136 to 104 on 64bit and from 76
> to 60 on 32bit with lock debugging turned off.  With lock debugging
> turned on the reduction is much larger.

Cool.

> s_active starts at zero and each active reference increments s_active.
> Putting a reference decrements s_active.  Deactivation subtracts
> SD_DEACTIVATED_BIAS which is currently INT_MIN and assumed to be small
> enough to make s_active negative.  If s_active is negative,
> sysfs_get() no longer grants new references.  Deactivation succeeds
> immediately if there is no active user; otherwise, it waits using a
> completion for the last put.
> 
> Due to the removal of lockdep tricks, this change makes things less
> trickier in release_sysfs_dirent().  As all the complexity is
> contained in three s_active functions, I think it's more readable this
> way.

Agreed.


> @@ -32,11 +33,24 @@ static DEFINE_IDA(sysfs_ino_ida);
>   */
>  struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
>  {
> -	if (sd) {
> -		if (unlikely(!down_read_trylock(&sd->s_active)))
> -			sd = NULL;
> +	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))
> +			return sd;
> +		if (t < 0)
> +			return NULL;
> +
> +		cpu_relax();
>  	}
> -	return sd;
>  }

I don't quite like v and t, but don't have a better naming suggestion
either.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/