[QUESTION] inconsistent use of smp_mb()

[QUESTION] inconsistent use of smp_mb()

[Zilin Guan]

Hello,

I have a question regarding the use of smp_rmb() to enforce 
memory ordering in two related functions.

In the function netfs_unbuffered_write_iter_locked() from the file 
fs/netfs/direct_write.c, smp_rmb() is explicitly used after the 
wait_on_bit() call to ensure that the error and transferred fields are 
read in the correct order following the NETFS_RREQ_IN_PROGRESS flag:

105	wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS,
106		    TASK_UNINTERRUPTIBLE);
107	smp_rmb(); /* Read error/transferred after RIP flag */
108	ret = wreq->error;
109	if (ret == 0) {
110		ret = wreq->transferred;
111		iocb->ki_pos += ret;
112	}

However, in the function netfs_end_writethrough() from the file 
fs/netfs/write_issue.c, there is no such use of smp_rmb() after 
the corresponding wait_on_bit() call, despite accessing the same filed 
of wreq->error and relying on the same NETFS_RREQ_IN_PROGRESS flag:

681	wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS, 
		    TASK_UNINTERRUPTIBLE);
682	ret = wreq->error;

My question is why does the first function require a CPU memory barrier 
smp_rmb() to enforce ordering, whereas the second function does not?

Thank you for your time and assistance!

Best Regards,
Zilin Guan

Re: [QUESTION] inconsistent use of smp_mb()

[Mateusz Guzik]

On Wed, Dec 04, 2024 at 06:48:18AM +0000, Zilin Guan wrote:
> Hello,
> 
> I have a question regarding the use of smp_rmb() to enforce 
> memory ordering in two related functions.
> 
> In the function netfs_unbuffered_write_iter_locked() from the file 
> fs/netfs/direct_write.c, smp_rmb() is explicitly used after the 
> wait_on_bit() call to ensure that the error and transferred fields are 
> read in the correct order following the NETFS_RREQ_IN_PROGRESS flag:
> 
> 105	wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS,
> 106		    TASK_UNINTERRUPTIBLE);
> 107	smp_rmb(); /* Read error/transferred after RIP flag */
> 108	ret = wreq->error;
> 109	if (ret == 0) {
> 110		ret = wreq->transferred;
> 111		iocb->ki_pos += ret;
> 112	}
> 
> However, in the function netfs_end_writethrough() from the file 
> fs/netfs/write_issue.c, there is no such use of smp_rmb() after 
> the corresponding wait_on_bit() call, despite accessing the same filed 
> of wreq->error and relying on the same NETFS_RREQ_IN_PROGRESS flag:
> 
> 681	wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS, 
> 		    TASK_UNINTERRUPTIBLE);
> 682	ret = wreq->error;
> 
> My question is why does the first function require a CPU memory barrier 
> smp_rmb() to enforce ordering, whereas the second function does not?

The fence is redundant.

Per the comment in wait_on_bit:
 * Returned value will be zero if the bit was cleared in which case the
 * call has ACQUIRE semantics, or %-EINTR if the process received a
 * signal and the mode permitted wake up on that signal.

Since both sites pass TASK_UNINTERRUPTIBLE this will only ever return
after the bit is sorted out, already providing the needed fence.

Re: [QUESTION] inconsistent use of smp_mb()

[Zilin Guan]

On Wed, Dec 04, 2024 at 09:27:22AM+0100, Mateusz Guzik wrote:
> On Wed, Dec 04, 2024 at 06:48:18AM +0000, Zilin Guan wrote:
> > Hello,
> > 
> > I have a question regarding the use of smp_rmb() to enforce 
> > memory ordering in two related functions.
> > 
> > In the function netfs_unbuffered_write_iter_locked() from the file 
> > fs/netfs/direct_write.c, smp_rmb() is explicitly used after the 
> > wait_on_bit() call to ensure that the error and transferred fields are 
> > read in the correct order following the NETFS_RREQ_IN_PROGRESS flag:
> > 
> > 105	wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS,
> > 106		    TASK_UNINTERRUPTIBLE);
> > 107	smp_rmb(); /* Read error/transferred after RIP flag */
> > 108	ret = wreq->error;
> > 109	if (ret == 0) {
> > 110		ret = wreq->transferred;
> > 111		iocb->ki_pos += ret;
> > 112	}
> > 
> > However, in the function netfs_end_writethrough() from the file 
> > fs/netfs/write_issue.c, there is no such use of smp_rmb() after 
> > the corresponding wait_on_bit() call, despite accessing the same filed 
> > of wreq->error and relying on the same NETFS_RREQ_IN_PROGRESS flag:
> > 
> > 681	wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS, 
> > 		    TASK_UNINTERRUPTIBLE);
> > 682	ret = wreq->error;
> > 
> > My question is why does the first function require a CPU memory barrier 
> > smp_rmb() to enforce ordering, whereas the second function does not?
> 
> The fence is redundant.
> 
> Per the comment in wait_on_bit:
>  * Returned value will be zero if the bit was cleared in which case the
>  * call has ACQUIRE semantics, or %-EINTR if the process received a
>  * signal and the mode permitted wake up on that signal.
> 
> Since both sites pass TASK_UNINTERRUPTIBLE this will only ever return
> after the bit is sorted out, already providing the needed fence.
 
Since the code does not need the fence, should I send a patch to 
remove it? Commit 2df8654 introduced this fence during the transition 
to a new writeback implementation. However, the author added this fence 
as part of the changes and did not intend to address a specific CPU 
reordering issue.

Re: [QUESTION] inconsistent use of smp_mb()

[David Howells]

Zilin Guan <zilin@seu.edu.cn> wrote:

> Since the code does not need the fence, should I send a patch to 
> remove it? Commit 2df8654 introduced this fence during the transition 
> to a new writeback implementation. However, the author added this fence 
> as part of the changes and did not intend to address a specific CPU 
> reordering issue.

Sure.

Thanks,
David