Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Shaokun Zhang <zhangshaokun@hisilicon.com>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	Yuqi Jin <jinyuqi@huawei.com>,
	kernel test robot <rong.a.chen@intel.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Boqun Feng <boqun.feng@gmail.com>
Subject: [NAK] Re: [PATCH] fs: Optimized fget to improve performance
Date: Thu, 27 Aug 2020 15:28:48 +0100	[thread overview]
Message-ID: <20200827142848.GZ1236603@ZenIV.linux.org.uk> (raw)
In-Reply-To: <1598523584-25601-1-git-send-email-zhangshaokun@hisilicon.com>

On Thu, Aug 27, 2020 at 06:19:44PM +0800, Shaokun Zhang wrote:
> From: Yuqi Jin <jinyuqi@huawei.com>
> 
> It is well known that the performance of atomic_add is better than that of
> atomic_cmpxchg.
> The initial value of @f_count is 1. While @f_count is increased by 1 in
> __fget_files, it will go through three phases: > 0, < 0, and = 0. When the
> fixed value 0 is used as the condition for terminating the increase of 1,
> only atomic_cmpxchg can be used. When we use < 0 as the condition for
> stopping plus 1, we can use atomic_add to obtain better performance.

Suppose another thread has just removed it from the descriptor table.

> +static inline bool get_file_unless_negative(atomic_long_t *v, long a)
> +{
> +	long c = atomic_long_read(v);
> +
> +	if (c <= 0)
> +		return 0;

Still 1.  Now the other thread has gotten to dropping the last reference,
decremented counter to zero and committed to freeing the struct file.

> +
> +	return atomic_long_add_return(a, v) - 1;

... and you increment that sucker back to 1.  Sure, you return 0, so the
caller does nothing to that struct file.  Which includes undoing the
changes to its refecount.

In the meanwhile, the third thread does fget on the same descriptor,
and there we end up bumping the refcount to 2 and succeeding.  Which
leaves the caller with reference to already doomed struct file...

	IOW, NAK - this is completely broken.  The whole point of
atomic_long_add_unless() is that the check and conditional increment
are atomic.  Together.  That's what your optimization takes out.

  parent reply	other threads:[~2020-08-27 14:29 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-27 10:19 Shaokun Zhang
2020-08-27 12:30 ` Matthew Wilcox
2020-08-27 13:07   ` David Laight
2020-08-27 14:28 ` Al Viro [this message]
2020-08-28 11:04   ` [NAK] " Will Deacon
2020-08-31  1:43   ` Shaokun Zhang
2020-08-31  3:21     ` Al Viro
2020-09-01  9:29       ` David Laight

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200827142848.GZ1236603@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=boqun.feng@gmail.com \
    --cc=jinyuqi@huawei.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=peterz@infradead.org \
    --cc=rong.a.chen@intel.com \
    --cc=will@kernel.org \
    --cc=zhangshaokun@hisilicon.com \
    --subject='[NAK] Re: [PATCH] fs: Optimized fget to improve performance' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).