Re: [PATCH v5 1/2] uprobes: Remove redundant spinlock in uprobe_deny_signal()

[Steven Rostedt]

On Fri, 24 Jan 2025 09:38:25 +0000
Liao Chang <liaochang1@xxxxxxxxxx> wrote:

> Since clearing a bit in thread_info is an atomic operation, the spinlock
> is redundant and can be removed, reducing lock contention is good for
> performance.

Although this patch is probably fine, the change log suggests a dangerous
precedence. Just because clearing a flag is atomic, that alone does not
guarantee that it doesn't need spin locks around it.

There may be another path that tests the flag within a spin lock, and then
does a bunch of work assuming that the flag does not change while it is
doing that work. That other path would require a spin lock around the
clearing of the flag elsewhere.

I don't know this code well enough to know if this has that scenario, and
seeing the Acked-by from Oleg, I'm assuming it does not. But in any case,
the change log needs to give a better rationale for removing a spin lock than
just "clearing a flag atomically doesn't need a spin lock"!

-- Steve


> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> Acked-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> Signed-off-by: Liao Chang <liaochang1@xxxxxxxxxx>