linux-kernel - Re: [linus:master] [fs] a914bd93f3: stress-ng.close.close_calls_per

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <hupjeobnfvo7y3jyvomjuqxtdl2df2myqxwr3bktmxabsrbid4@erg2ghyfkuj5>
Date: Thu, 17 Apr 2025 12:17:54 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: kernel test robot <oliver.sang@...el.com>
Cc: oe-lkp@...ts.linux.dev, lkp@...el.com, linux-kernel@...r.kernel.org, 
	Christian Brauner <brauner@...nel.org>, linux-fsdevel@...r.kernel.org
Subject: Re: [linus:master] [fs] a914bd93f3:
 stress-ng.close.close_calls_per_sec 52.2% regression

On Thu, Apr 17, 2025 at 12:02:55PM +0200, Mateusz Guzik wrote:
> bottom line though is there is a known tradeoff there and stress-ng
> manufactures a case where it is always the wrong one.
> 
> fd 2 at hand is inherited (it's the tty) and shared between *all*
> workers on all CPUs.
> 
> Ignoring some fluff, it's this in a loop:
> dup2(2, 1024)                           = 1024
> dup2(2, 1025)                           = 1025
> dup2(2, 1026)                           = 1026
> dup2(2, 1027)                           = 1027
> dup2(2, 1028)                           = 1028
> dup2(2, 1029)                           = 1029
> dup2(2, 1030)                           = 1030
> dup2(2, 1031)                           = 1031
> [..]
> close_range(1024, 1032, 0)              = 0
> 
> where fd 2 is the same file object in all 192 workers doing this.
> 

the following will still have *some* impact, but the drop should be much
lower

it also has a side effect of further helping the single-threaded case by
shortening the code when it works

diff --git a/include/linux/file_ref.h b/include/linux/file_ref.h
index 7db62fbc0500..c73865ed4251 100644
--- a/include/linux/file_ref.h
+++ b/include/linux/file_ref.h
@@ -181,17 +181,15 @@ static __always_inline __must_check bool file_ref_put_close(file_ref_t *ref)
 	long old, new;
 
 	old = atomic_long_read(&ref->refcnt);
-	do {
-		if (unlikely(old < 0))
-			return __file_ref_put_badval(ref, old);
-
-		if (old == FILE_REF_ONEREF)
-			new = FILE_REF_DEAD;
-		else
-			new = old - 1;
-	} while (!atomic_long_try_cmpxchg(&ref->refcnt, &old, new));
-
-	return new == FILE_REF_DEAD;
+	if (likely(old == FILE_REF_ONEREF)) {
+		new = FILE_REF_DEAD;
+		if (likely(atomic_long_try_cmpxchg(&ref->refcnt, &old, new)))
+			return true;
+		/*
+		 * The ref has changed from under us, don't play any games.
+		 */
+	}
+	return file_ref_put(ref);
 }
 
 /**