| Message ID | 20240919081432.23431-1-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: Use try_cmpxchg() in xlog_cil_insert_pcp_aggregate() | expand |
On Thu, Sep 19, 2024 at 10:14:05AM +0200, Uros Bizjak wrote: > --- a/fs/xfs/xfs_log_cil.c > +++ b/fs/xfs/xfs_log_cil.c > @@ -171,13 +171,12 @@ xlog_cil_insert_pcp_aggregate( > * structures that could have a nonzero space_used. > */ > for_each_cpu(cpu, &ctx->cil_pcpmask) { > - int old, prev; > + int old; > > cilpcp = per_cpu_ptr(cil->xc_pcp, cpu); > + old = READ_ONCE(cilpcp->space_used); Maybe it is just me, but this would probably look nicer if the cilpcp variable moved into the loop scope, and both were initialized at declaration time: struct xlog_cil_pcp *cilpcp = per_cpu_ptr(cil->xc_pcp, cpu); int old = READ_ONCE(cilpcp->space_used); > do { > + } while (!try_cmpxchg(&cilpcp->space_used, &old, 0)); And this also looks a bit odd. Again, probably preference, but a: while (!try_cmpxchg(&cilpcp->space_used, &old, 0)) ; looks somewhat more normal (although still not pretty). Sorry for not having anything more substantial to see, but the diff just looked a bit odd..
On Fri, Sep 20, 2024 at 4:16 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Sep 19, 2024 at 10:14:05AM +0200, Uros Bizjak wrote: > > --- a/fs/xfs/xfs_log_cil.c > > +++ b/fs/xfs/xfs_log_cil.c > > @@ -171,13 +171,12 @@ xlog_cil_insert_pcp_aggregate( > > * structures that could have a nonzero space_used. > > */ > > for_each_cpu(cpu, &ctx->cil_pcpmask) { > > - int old, prev; > > + int old; > > > > cilpcp = per_cpu_ptr(cil->xc_pcp, cpu); > > + old = READ_ONCE(cilpcp->space_used); > > Maybe it is just me, but this would probably look nicer if the cilpcp > variable moved into the loop scope, and both were initialized at > declaration time: > > struct xlog_cil_pcp *cilpcp = per_cpu_ptr(cil->xc_pcp, cpu); > int old = READ_ONCE(cilpcp->space_used); No problem, I just tried to keep the number of changed lines as low as possible. Some maintainers don't like functional and cosmetic changes mixed together. > > > do { > > + } while (!try_cmpxchg(&cilpcp->space_used, &old, 0)); > > And this also looks a bit odd. Again, probably preference, but a: > > while (!try_cmpxchg(&cilpcp->space_used, &old, 0)) > ; > > looks somewhat more normal (although still not pretty). Yes, the alternative form is what some maintainers prefer. > Sorry for not having anything more substantial to see, but the diff > just looked a bit odd.. I'll prepare a v2 patch with the suggested changes. Thanks, Uros.
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c index 391a938d690c..e7a9fcd6935b 100644 --- a/fs/xfs/xfs_log_cil.c +++ b/fs/xfs/xfs_log_cil.c @@ -171,13 +171,12 @@ xlog_cil_insert_pcp_aggregate( * structures that could have a nonzero space_used. */ for_each_cpu(cpu, &ctx->cil_pcpmask) { - int old, prev; + int old; cilpcp = per_cpu_ptr(cil->xc_pcp, cpu); + old = READ_ONCE(cilpcp->space_used); do { - old = cilpcp->space_used; - prev = cmpxchg(&cilpcp->space_used, old, 0); - } while (old != prev); + } while (!try_cmpxchg(&cilpcp->space_used, &old, 0)); count += old; } atomic_add(count, &ctx->space_used);
Use !try_cmpxchg instead of cmpxchg (*ptr, old, new) != old in xlog_cil_insert_pcp_aggregate(). x86 CMPXCHG instruction returns success in ZF flag, so this change saves a compare after cmpxchg. Also, try_cmpxchg implicitly assigns old *ptr value to "old" when cmpxchg fails. There is no need to re-read the value in the loop. Note that the value from *ptr should be read using READ_ONCE to prevent the compiler from merging, refetching or reordering the read. No functional change intended. Signed-off-by: Uros Bizjak <ubizjak@gmail.com> Cc: Chandan Babu R <chandan.babu@oracle.com> Cc: "Darrick J. Wong" <djwong@kernel.org> --- fs/xfs/xfs_log_cil.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)