Received-SPF: pass (google.com: domain of linux-kernel+bounces-13591-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <73a4b31c9c874081baabad9e5f2e5204@AcuMS.aculab.com> <bddb6b00434d4492abca4725c10f8d5a@AcuMS.aculab.com>
In-Reply-To: <bddb6b00434d4492abca4725c10f8d5a@AcuMS.aculab.com>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 30 Dec 2023 12:41:12 -0800
Message-ID: <CAHk-=wjbWTbRKDP=Yb9VWBGjSBEGB3dJ0=--+7-4oA2n1=1FKw@mail.gmail.com>
Subject: Re: [PATCH next 4/5] locking/osq_lock: Optimise per-cpu data accesses.
To: David Laight <David.Laight@aculab.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
	"peterz@infradead.org" <peterz@infradead.org>, "longman@redhat.com" <longman@redhat.com>, 
	"mingo@redhat.com" <mingo@redhat.com>, "will@kernel.org" <will@kernel.org>, 
	"boqun.feng@gmail.com" <boqun.feng@gmail.com>, 
	"xinhui.pan@linux.vnet.ibm.com" <xinhui.pan@linux.vnet.ibm.com>, 
	"virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org>, 
	Zeng Heng <zengheng4@huawei.com>
Content-Type: multipart/mixed; boundary="000000000000641af2060dc02c7b"

--000000000000641af2060dc02c7b
Content-Type: text/plain; charset="UTF-8"

On Fri, 29 Dec 2023 at 12:57, David Laight <David.Laight@aculab.com> wrote:
>
> this_cpu_ptr() is rather more expensive than raw_cpu_read() since
> the latter can use an 'offset from register' (%gs for x86-84).
>
> Add a 'self' field to 'struct optimistic_spin_node' that can be
> read with raw_cpu_read(), initialise on first call.

No, this is horrible.

The problem isn't the "this_cpu_ptr()", it's the rest of the code.

>  bool osq_lock(struct optimistic_spin_queue *lock)
>  {
> -       struct optimistic_spin_node *node = this_cpu_ptr(&osq_node);
> +       struct optimistic_spin_node *node = raw_cpu_read(osq_node.self);

No. Both of these are crap.

>         struct optimistic_spin_node *prev, *next;
>         int old;
>
> -       if (unlikely(node->cpu == OSQ_UNLOCKED_VAL))
> -               node->cpu = encode_cpu(smp_processor_id());
> +       if (unlikely(!node)) {
> +               int cpu = encode_cpu(smp_processor_id());
> +               node = decode_cpu(cpu);
> +               node->self = node;
> +               node->cpu = cpu;
> +       }

The proper fix here is to not do that silly

        node = this_cpu_ptr(&osq_node);
        ..
        node->next = NULL;

dance at all, but to simply do

        this_cpu_write(osq_node.next, NULL);

in the first place. That makes the whole thing just a single store off
the segment descriptor.

Yes, you'll eventually end up doing that

        node = this_cpu_ptr(&osq_node);

thing because it then wants to use that raw pointer to do

        WRITE_ONCE(prev->next, node);

but that's a separate issue and still does not make it worth it to
create a pointless self-pointer.

Btw, if you *really* want to solve that separate issue, then make the
optimistic_spin_node struct not contain the pointers at all, but the
CPU numbers, and then turn those numbers into the pointers the exact
same way it does for the "lock->tail" thing, ie doing that whole

        prev = decode_cpu(old);

dance. That *may* then result in avoiding turning them into pointers
at all in some cases.

Also, I think that you might want to look into making OSQ_UNLOCKED_VAL
be -1 instead, and add something like

  #define IS_OSQ_UNLOCKED(x) ((int)(x)<0)

and that would then avoid the +1 / -1 games in encoding/decoding the
CPU numbers. It causes silly code generated like this:

        subl    $1, %eax        #, cpu_nr
...
        cltq
        addq    __per_cpu_offset(,%rax,8), %rcx

which seems honestly stupid. The cltq is there for sign-extension,
which is because all these things are "int", and the "subl" will
zero-extend to 64-bit, not sign-extend.

At that point, I think gcc might be able to just generate

        addq    __per_cpu_offset-8(,%rax,8), %rcx

but honestly, I think it would be nicer to just have decode_cpu() do

        unsigned int cpu_nr = encoded_cpu_val;

        return per_cpu_ptr(&osq_node, cpu_nr);

and not have the -1/+1 at all.

Hmm?

UNTESTED patch to just do the "this_cpu_write()" parts attached.
Again, note how we do end up doing that this_cpu_ptr conversion later
anyway, but at least it's off the critical path.

                 Linus

--000000000000641af2060dc02c7b
Content-Type: text/x-patch; charset="US-ASCII"; name="patch.diff"
Content-Disposition: attachment; filename="patch.diff"
Content-Transfer-Encoding: base64
Content-ID: <f_lqsj1wd50>
X-Attachment-Id: f_lqsj1wd50

IGtlcm5lbC9sb2NraW5nL29zcV9sb2NrLmMgfCAxMiArKysrKysrLS0tLS0KIDEgZmlsZSBjaGFu
Z2VkLCA3IGluc2VydGlvbnMoKyksIDUgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEva2VybmVs
L2xvY2tpbmcvb3NxX2xvY2suYyBiL2tlcm5lbC9sb2NraW5nL29zcV9sb2NrLmMKaW5kZXggNzVh
NmY2MTMzODY2Li5jM2ExNjZiNzkwMGMgMTAwNjQ0Ci0tLSBhL2tlcm5lbC9sb2NraW5nL29zcV9s
b2NrLmMKKysrIGIva2VybmVsL2xvY2tpbmcvb3NxX2xvY2suYwpAQCAtOTIsMTQgKzkyLDE0IEBA
IG9zcV93YWl0X25leHQoc3RydWN0IG9wdGltaXN0aWNfc3Bpbl9xdWV1ZSAqbG9jaywKIAogYm9v
bCBvc3FfbG9jayhzdHJ1Y3Qgb3B0aW1pc3RpY19zcGluX3F1ZXVlICpsb2NrKQogewotCXN0cnVj
dCBvcHRpbWlzdGljX3NwaW5fbm9kZSAqbm9kZSA9IHRoaXNfY3B1X3B0cigmb3NxX25vZGUpOwor
CXN0cnVjdCBvcHRpbWlzdGljX3NwaW5fbm9kZSAqbm9kZTsKIAlzdHJ1Y3Qgb3B0aW1pc3RpY19z
cGluX25vZGUgKnByZXYsICpuZXh0OwogCWludCBjdXJyID0gZW5jb2RlX2NwdShzbXBfcHJvY2Vz
c29yX2lkKCkpOwogCWludCBvbGQ7CiAKLQlub2RlLT5sb2NrZWQgPSAwOwotCW5vZGUtPm5leHQg
PSBOVUxMOwotCW5vZGUtPmNwdSA9IGN1cnI7CisJdGhpc19jcHVfd3JpdGUob3NxX25vZGUubmV4
dCwgTlVMTCk7CisJdGhpc19jcHVfd3JpdGUob3NxX25vZGUubG9ja2VkLCAwKTsKKwl0aGlzX2Nw
dV93cml0ZShvc3Ffbm9kZS5jcHUsIGN1cnIpOwogCiAJLyoKIAkgKiBXZSBuZWVkIGJvdGggQUNR
VUlSRSAocGFpcnMgd2l0aCBjb3JyZXNwb25kaW5nIFJFTEVBU0UgaW4KQEAgLTExMiw3ICsxMTIs
OSBAQCBib29sIG9zcV9sb2NrKHN0cnVjdCBvcHRpbWlzdGljX3NwaW5fcXVldWUgKmxvY2spCiAJ
CXJldHVybiB0cnVlOwogCiAJcHJldiA9IGRlY29kZV9jcHUob2xkKTsKLQlub2RlLT5wcmV2ID0g
cHJldjsKKwl0aGlzX2NwdV93cml0ZShvc3Ffbm9kZS5wcmV2LCBwcmV2KTsKKworCW5vZGUgPSB0
aGlzX2NwdV9wdHIoJm9zcV9ub2RlKTsKIAogCS8qCiAJICogb3NxX2xvY2soKQkJCXVucXVldWUK
--000000000000641af2060dc02c7b--