fs/netfs: Remove redundant use of smp_rmb()

Commit Message

Zilin Guan Dec. 7, 2024, 2:19 a.m. UTC

The function netfs_unbuffered_write_iter_locked() in
fs/netfs/direct_write.c contains an unnecessary smp_rmb() call after
wait_on_bit(). Since wait_on_bit() already incorporates a memory barrier
that ensures the flag update is visible before the function returns, the
smp_rmb() provides no additional benefit and incurs unnecessary overhead.

This patch removes the redundant barrier to simplify and optimize the code.

Signed-off-by: Zilin Guan <[e-mail]>
---
 fs/netfs/direct_write.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Akira Yokosawa Dec. 7, 2024, 5:20 a.m. UTC | #1

[+CC: Mateusz, who responded ZiLin's original question at:
  https://lore.kernel.org/ulg54pf2qnlzqfj247fypypzun2yvwepqrcwaqzlr6sn3ukuab@rov7btfppktc/ ]

On Sat,  7 Dec 2024 02:19:52 +0000, Zilin Guan wrote:
> The function netfs_unbuffered_write_iter_locked() in
> fs/netfs/direct_write.c contains an unnecessary smp_rmb() call after
> wait_on_bit(). Since wait_on_bit() already incorporates a memory barrier
> that ensures the flag update is visible before the function returns, the
> smp_rmb() provides no additional benefit and incurs unnecessary overhead.
> 
> This patch removes the redundant barrier to simplify and optimize the code.
> 
> Signed-off-by: Zilin Guan <[e-mail]>
> ---
>  fs/netfs/direct_write.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/netfs/direct_write.c b/fs/netfs/direct_write.c
> index 88f2adfab75e..173e8b5e6a93 100644
> --- a/fs/netfs/direct_write.c
> +++ b/fs/netfs/direct_write.c
> @@ -104,7 +104,6 @@ ssize_t netfs_unbuffered_write_iter_locked(struct kiocb *iocb, struct iov_iter *
>  		trace_netfs_rreq(wreq, netfs_rreq_trace_wait_ip);
>  		wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS,
>  			    TASK_UNINTERRUPTIBLE);
> -		smp_rmb(); /* Read error/transferred after RIP flag */
>  		ret = wreq->error;
>  		if (ret == 0) {
>  			ret = wreq->transferred;

You are removing a barrier which is deemed to be required by LKMM.
See section "SLEEP AND WAKE-UP FUNCTIONS" in Documentation/memory-barriers.txt.

Quoting relevant note below:

-----------------------------------------------------------------------------

[!] Note that the memory barriers implied by the sleeper and the waker do *not*
order multiple stores before the wake-up with respect to loads of those stored
values after the sleeper has called set_current_state().  For instance, if the
sleeper does:

	set_current_state(TASK_INTERRUPTIBLE);
	if (event_indicated)
		break;
	__set_current_state(TASK_RUNNING);
	do_something(my_data);

and the waker does:

	my_data = value;
	event_indicated = 1;
	wake_up(&event_wait_queue);

there's no guarantee that the change to event_indicated will be perceived by
the sleeper as coming after the change to my_data.  In such a circumstance, the
code on both sides must interpolate its own memory barriers between the
separate data accesses.  Thus the above sleeper ought to do:

	set_current_state(TASK_INTERRUPTIBLE);
	if (event_indicated) {
		smp_rmb();
		do_something(my_data);
	}

and the waker should do:

	my_data = value;
	smp_wmb();
	event_indicated = 1;
	wake_up(&event_wait_queue);

-----------------------------------------------------------------------------

Are you sure removing the smp_rmb() is realy the right thing to do?

        Thanks, Akira

> -- 
> 2.34.1

David Howells Dec. 7, 2024, 8:04 a.m. UTC | #2

Akira Yokosawa <[e-mail]> wrote:

> Are you sure removing the smp_rmb() is realy the right thing to do?

The wait_on_bit*() class functions, e.g.:

	wait_on_bit(unsigned long *word, int bit, unsigned mode)
	{
		might_sleep();
		if (!test_bit_acquire(bit, word))
			return 0;
		return out_of_line_wait_on_bit(word, bit,
					       bit_wait,
					       mode);
	}

now unconditionally includes an appropriate barrier on the test_bit(), so the
smp_rmb() should be unnecessary, though netfslib should probably be using
clear_and_wake_up_bit().

Probably we need to update the doc to reflect this.

David

Akira Yokosawa Dec. 7, 2024, 10:09 a.m. UTC | #3

Hi David,

On Sat, 07 Dec 2024 08:04:56 +0000, David Howells wrote:
> Akira Yokosawa <[e-mail]> wrote:
> 
>> Are you sure removing the smp_rmb() is realy the right thing to do?
> 
> The wait_on_bit*() class functions, e.g.:
> 
> 	wait_on_bit(unsigned long *word, int bit, unsigned mode)
> 	{
> 		might_sleep();
> 		if (!test_bit_acquire(bit, word))
> 			return 0;
> 		return out_of_line_wait_on_bit(word, bit,
> 					       bit_wait,
> 					       mode);
> 	}
> 
> now unconditionally includes an appropriate barrier on the test_bit(), so the
> smp_rmb() should be unnecessary, though netfslib should probably be using
> clear_and_wake_up_bit().
> 

Thank you for clarifying.

> Probably we need to update the doc to reflect this.

Agreed.

I see that wait_on_bit()'s kernel-doc comment mentions implicit ACQUIRE
semantics on success, and that of wake_up_bit() mentions the need of care
for memory ordering before calling it.

Unfortunately, neither of those comments is included into kernel
documentation build (Sphinx) at the moment.

I'm going to prepare a patch for including them somewhere under the
core-api doc.

WRT memory-barriers.txt, I'm not sure I can update it properly.

David, may I ask you doing that part?

Thanks, Akira

Patch

diff --git a/fs/netfs/direct_write.c b/fs/netfs/direct_write.c
index 88f2adfab75e..173e8b5e6a93 100644
--- a/fs/netfs/direct_write.c
+++ b/fs/netfs/direct_write.c
@@ -104,7 +104,6 @@  ssize_t netfs_unbuffered_write_iter_locked(struct kiocb *iocb, struct iov_iter *
 		trace_netfs_rreq(wreq, netfs_rreq_trace_wait_ip);
 		wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS,
 			    TASK_UNINTERRUPTIBLE);
-		smp_rmb(); /* Read error/transferred after RIP flag */
 		ret = wreq->error;
 		if (ret == 0) {
 			ret = wreq->transferred;