lkml.org 
[lkml]   [2023]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH 3/3] ring_buffer: Use try_cmpxchg instead of cmpxchg
    On Tue, Feb 28, 2023 at 10:43 PM Steven Rostedt <rostedt@goodmis.org> wrote:
    >
    > On Tue, 28 Feb 2023 18:59:29 +0100
    > Uros Bizjak <ubizjak@gmail.com> wrote:
    >
    > > Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old.
    > > x86 CMPXCHG instruction returns success in ZF flag, so this change
    > > saves a compare after cmpxchg (and related move instruction in
    > > front of 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.
    > >
    > > No functional change intended.
    >
    > As I mentioned in the RCU thread, I have issues with some of the changes
    > here.
    >
    > >
    > > Cc: Steven Rostedt <rostedt@goodmis.org>
    > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
    > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
    > > ---
    > > kernel/trace/ring_buffer.c | 20 ++++++++------------
    > > 1 file changed, 8 insertions(+), 12 deletions(-)
    > >
    > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
    > > index 4188af7d4cfe..8f0ef7d12ddd 100644
    > > --- a/kernel/trace/ring_buffer.c
    > > +++ b/kernel/trace/ring_buffer.c
    > > @@ -1493,14 +1493,11 @@ static bool rb_head_page_replace(struct buffer_page *old,
    > > {
    > > unsigned long *ptr = (unsigned long *)&old->list.prev->next;
    > > unsigned long val;
    > > - unsigned long ret;
    > >
    > > val = *ptr & ~RB_FLAG_MASK;
    > > val |= RB_PAGE_HEAD;
    > >
    > > - ret = cmpxchg(ptr, val, (unsigned long)&new->list);
    > > -
    > > - return ret == val;
    > > + return try_cmpxchg(ptr, &val, (unsigned long)&new->list);
    >
    > No, val should not be updated.

    Please see the definition of try_cmpxchg. The definition is written in
    such a way that benefits loops as well as linear code and in the later
    case depends on the compiler to eliminate assignment to val as a dead
    assignment.

    The above change was done under the assumption that val is unused
    after try_cmpxchg, and can be considered as a temporary
    [Alternatively, the value could be copied to a local temporary and a
    pointer to this local temporary could be passed to try_cmpxchg
    instead. Compiler is smart enough to eliminate the assignment in any
    case.]

    Even in the linear code, the change has considerable effect.
    rb_head_page_replace is inlined in rb_get_reader_page and gcc-10.3.1
    improves code from:

    ef8: 48 8b 0e mov (%rsi),%rcx
    efb: 48 83 e1 fc and $0xfffffffffffffffc,%rcx
    eff: 48 83 c9 01 or $0x1,%rcx
    f03: 48 89 c8 mov %rcx,%rax
    f06: f0 48 0f b1 3e lock cmpxchg %rdi,(%rsi)
    f0b: 48 39 c1 cmp %rax,%rcx
    f0e: 74 3b je f4b <rb_get_reader_page+0x13b>

    to:

    ed8: 48 8b 01 mov (%rcx),%rax
    edb: 48 83 e0 fc and $0xfffffffffffffffc,%rax
    edf: 48 83 c8 01 or $0x1,%rax
    ee3: f0 48 0f b1 31 lock cmpxchg %rsi,(%rcx)
    ee8: 74 3b je f25 <rb_get_reader_page+0x135>

    Again, even in linear code the change is able to eliminate the
    assignment to a temporary reg and the compare. Please note that there
    is no move *from* %rax register after cmpxchg, so the compiler
    correctly eliminated unused assignment.

    >
    > > }
    > >
    > > /*
    > > @@ -2055,7 +2052,7 @@ rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer)
    > > retries = 10;
    > > success = false;
    > > while (retries--) {
    > > - struct list_head *head_page, *prev_page, *r;
    > > + struct list_head *head_page, *prev_page;
    > > struct list_head *last_page, *first_page;
    > > struct list_head *head_page_with_bit;
    > >
    > > @@ -2073,9 +2070,8 @@ rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer)
    > > last_page->next = head_page_with_bit;
    > > first_page->prev = prev_page;
    > >
    > > - r = cmpxchg(&prev_page->next, head_page_with_bit, first_page);
    > > -
    > > - if (r == head_page_with_bit) {
    > > + if (try_cmpxchg(&prev_page->next,
    > > + &head_page_with_bit, first_page)) {
    >
    > No. head_page_with_bit should not be updated.

    As above, head_page_with_bit should be considered as a temporary, it
    is initialized a couple of lines above cmpxchg and unused after. The
    gcc-10.3.1 compiler even found some more optimization opportunities
    and reordered the code from:

    1364: 4d 8b 86 38 01 00 00 mov 0x138(%r14),%r8
    136b: 48 83 ce 01 or $0x1,%rsi
    136f: 48 89 f0 mov %rsi,%rax
    1372: 49 89 30 mov %rsi,(%r8)
    1375: 48 89 4f 08 mov %rcx,0x8(%rdi)
    1379: f0 48 0f b1 39 lock cmpxchg %rdi,(%rcx)
    137e: 48 39 c6 cmp %rax,%rsi
    1381: 74 78 je 13fb <rb_insert_pages+0xdb>
    to:

    1343: 48 83 c8 01 or $0x1,%rax
    1347: 48 8b bb 38 01 00 00 mov 0x138(%rbx),%rdi
    134e: 48 89 07 mov %rax,(%rdi)
    1351: 48 89 4e 08 mov %rcx,0x8(%rsi)
    1355: f0 48 0f b1 31 lock cmpxchg %rsi,(%rcx)
    135a: 41 0f 94 c7 sete %r15b
    135e: 75 2f jne 138f <rb_insert_pages+0x8f>

    Please also note SETE insn in the above code, this is how the
    "success" variable is handled in the loop. So, besides code size
    improvement, other secondary improvements can be expected from the
    change, too.

    I think that the above examples demonstrate various improvements that
    can be achieved with effectively a one-line, almost mechanical change
    to the code, even in linear code. It would be unfortunate to not
    consider them.

    Uros.

    \
     
     \ /
      Last update: 2023-03-27 00:39    [W:6.453 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site