linux-kernel - Re: [PATCH] uprobes: Improve the usage of xol slots for better scalability

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]

Message-ID: <ZvH_LiUeOtAwommF@tassilo>
Date: Mon, 23 Sep 2024 16:52:14 -0700
From: Andi Kleen <ak@...ux.intel.com>
To: "Liao, Chang" <liaochang1@...wei.com>
Cc: mhiramat@...nel.org, oleg@...hat.com, andrii@...nel.org,
	peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
	namhyung@...nel.org, mark.rutland@....com,
	alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
	irogers@...gle.com, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com, linux-kernel@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org,
	linux-perf-users@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH] uprobes: Improve the usage of xol slots for better
 scalability

> Thanks for the suggestions, I will experiment with a read-write lock, meanwhile,
> adding the documentation and testing for the lockless scheme.

Read-write locks are usually not worth it for short critical sections,
in fact they can be slower due to cache line effects.

> Sorry, I may not probably get the point clear here, and it would be very
> nice if more details are provided for the concern. Do you mean it's necessary
> to make the if-body excution exclusive among the CPUs? If that's the case,
> I guess the test_and_put_task_slot() is the equvialent to the race condition
> check. test_and_put_task_slot() uses a compare and exchange operation on the
> slot_ref of utask instance. Regardless of the work type being performed by
> other CPU, it will always bail out unless the slot_ref has a value of one,
> indicating the utask is free to access from local CPU.

What I meant is that the typical pattern for handling races in destruction
is to detect someone else is racing and then let it do the destruction
work or reacquire the resource (so just bail out).

But that's not what you're doing here, in fact you're adding a
completely new code path that has different semantics? I haven't checked
all the code, but it looks dubious.

-Andi