linux-kernel - Re: [PATCH v3 3/5] locking/qspinlock: Introduce CNA into the slow path of qspinlock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 Jul 2019 10:44:06 -0400
From:   Waiman Long <longman@...hat.com>
To:     Alex Kogan <alex.kogan@...cle.com>
Cc:     linux@...linux.org.uk, peterz@...radead.org, mingo@...hat.com,
        will.deacon@....com, arnd@...db.de, linux-arch@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        tglx@...utronix.de, bp@...en8.de, hpa@...or.com, x86@...nel.org,
        guohanjun@...wei.com, jglauber@...vell.com,
        steven.sistare@...cle.com, daniel.m.jordan@...cle.com,
        dave.dice@...cle.com, rahul.x.yadav@...cle.com
Subject: Re: [PATCH v3 3/5] locking/qspinlock: Introduce CNA into the slow
 path of qspinlock

On 7/16/19 10:26 AM, Alex Kogan wrote:
>> On Jul 15, 2019, at 5:30 PM, Waiman Long <longman@...hat.com> wrote:
>>
>> On 7/15/19 3:25 PM, Alex Kogan wrote:
>>
>>> /*
>>> - * On 64-bit architectures, the mcs_spinlock structure will be 16 bytes in
>>> - * size and four of them will fit nicely in one 64-byte cacheline. For
>>> - * pvqspinlock, however, we need more space for extra data. To accommodate
>>> - * that, we insert two more long words to pad it up to 32 bytes. IOW, only
>>> - * two of them can fit in a cacheline in this case. That is OK as it is rare
>>> - * to have more than 2 levels of slowpath nesting in actual use. We don't
>>> - * want to penalize pvqspinlocks to optimize for a rare case in native
>>> - * qspinlocks.
>>> + * On 64-bit architectures, the mcs_spinlock structure will be 20 bytes in
>>> + * size. For pvqspinlock or the NUMA-aware variant, however, we need more
>>> + * space for extra data. To accommodate that, we insert two more long words
>>> + * to pad it up to 36 bytes.
>>>  */
>> The 20 bytes figure is wrong. It is actually 24 bytes for 64-bit as the
>> mcs_spinlock structure is 8-byte aligned. For better cacheline
>> alignment, I will like to keep mcs_spinlock to 16 bytes as before.
>> Instead, you can use encode_tail() to store the CNA node pointer in
>> "locked". For instance, use (encode_tail() << 1) in locked to
>> distinguish it from the regular locked=1 value.
> I think this can work.
> decode_tail() will get the actual node pointer from the encoded value.
> And that would keep the size of mcs_spinlock intact.
> Good idea, thanks!
>
> BTW, maybe better change those function names to encode_node() / decode_node() then?

The names look good to me.


>
>>> s
>>> +
>>> +static void cna_init_node(struct mcs_spinlock *node)
>>> +{
>>> +	struct cna_node *cn = CNA_NODE(node);
>>> +	struct mcs_spinlock *base_node;
>>> +	int cpuid;
>>> +
>>> +	BUILD_BUG_ON(sizeof(struct cna_node) > sizeof(struct qnode));
>>> +	/* we store a pointer in the node's @locked field */
>>> +	BUILD_BUG_ON(sizeof(uintptr_t) > sizeof_field(struct mcs_spinlock, locked));
>>> +
>>> +	cpuid = smp_processor_id();
>>> +	cn->numa_node = cpu_to_node(cpuid);
>>> +
>>> +	base_node = this_cpu_ptr(&qnodes[0].mcs);
>>> +	cn->encoded_tail = encode_tail(cpuid, base_node->count - 1);
>>> +}
>>
>> I think you can use an early_init call to initialize the numa_node and
>> encoded_tail values for all the per-cpu CNA nodes instead of doing it
>> every time a node is used. If it turns out that pv_qspinlock is used,
>> the pv_node_init() will properly re-initialize it.
> Yes, this should work. Thanks.
>
> BTW, should not we initialize `cpu` in pv_init_node() that same way?

We would also initialize cpu this way in pv_init_node. The
smp_processor_id() call is relatively cheap, but the initialization done
here is more expensive.


>> The only thing left
>> to do here is perhaps setting tail to NULL.
> There is no need to initialize cna_node.tail — we never access it unless
> the node is at the head of the secondary queue, and in that case we 
> initialize it before placing the node at the head of that queue 
> (see find_successor()).

OK.

-Longman