public inbox for [email protected]
 help / color / mirror / Atom feed
From: Mathieu Desnoyers <[email protected]>
To: rostedt <[email protected]>
Cc: linux-kernel <[email protected]>,
	linux-trace-devel <[email protected]>,
	Ingo Molnar <[email protected]>,
	Andrew Morton <[email protected]>,
	Stefan Metzmacher <[email protected]>,
	io-uring <[email protected]>,
	Peter Zijlstra <[email protected]>,
	paulmck <[email protected]>
Subject: Re: [PATCH] tracepoints: Update static_call before tp_funcs when adding a tracepoint
Date: Mon, 26 Jul 2021 15:12:03 -0400 (EDT)	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

----- On Jul 26, 2021, at 2:49 PM, rostedt [email protected] wrote:

> On Mon, 26 Jul 2021 13:39:18 -0400 (EDT)
> Mathieu Desnoyers <[email protected]> wrote:
> 
>> ----- On Jul 26, 2021, at 12:56 PM, rostedt [email protected] wrote:
>> 
>> > On Mon, 26 Jul 2021 11:46:41 -0400 (EDT)
>> > Mathieu Desnoyers <[email protected]> wrote:
>> [...]
>> >   

[...]

>> 
>> AFAIU, none of the synchronization mechanisms you refer to here (memory barrier,
>> IPIs..) will change the fact that this CPU may still be delayed across the
>> entire
>> 1->0->1 transition sequence, and may end up calling the new callback with the
>> old data. Unless an explicit RCU-sync is done.
> 
> OK. I see the issue you are saying. And this came from my assumption
> that the tracepoint code did a synchronization when unregistering the
> last callback. But of course it wont because that would make a lot of
> back to back synchronizations of a large number of tracepoints being
> unregistered at once.
> 

Something along the lines of the work approach you propose should work.

>> 
>> >   
>> >> 
>> >> My third conclusion is that we'd need synchronize RCU whenever tp_funcs[0].data
>> >> changes for transitions 1->2, 2->1, and 1->2 because the priorities don't
>> >> guarantee
>> >> that the first callback stays in the first position, and we also need to rcu
>> >> sync
>> >> unconditionally on transition 1->0. We currently only have sync RCU on
>> >> transition
>> >> from 2->1 when tp_funcs[0].func changes, which is bogus in many ways.
>> > 
>> > Going from 1 to 2, there's no issue. We switch to the iterator, which
>> > is the old method anyway. It looks directly at the array and matches
>> > the data with the func for each element of that array, and the data
>> > read initially (before calling the iterator) is ignored.
>> 
>> This relies on ordering guarantees between RCU assign/dereference and
>> static_call
>> updates/call. It may well be the case, but I'm asking anyway.
>> 
>> Are we guaranteed of the following ordering ?
>> 
>> CPU A                             CPU B
>> 
>>                                   static_call_update()
> 
> The static_call_update() triggers an IPI on all CPUs that perform a full memory
> barrier.
> 
> That is, nothing on any CPU will cross the static_call_update().
> 
>> y = rcu_dereference(x)            rcu_assign_pointer(x, ...)
>> do_static_call(y)
>> 
>> That load of "x" should never happen after the CPU fetches the new static call
>> instruction.
> 
> The 'y' will always be the new static call (which is the iterator in
> this case), and it doesn't matter which x it read, because the iterator
> will read the array just like it was done without static calls.

Here by "y" I mean the pointer loaded by rcu_dereference, which is then passed to the
call. I do not mean the call per se.

I suspect there is something like a control dependency between loading "x" through
rcu_dereference and passing the loaded pointer as argument to the static call (and
the instruction fetch of the static call). But I don't recall reading any documentation
of this specific ordering.

> 
>> 
>> Also, I suspect that transition 2->1 needs an unconditional rcu-sync because you
>> may have a sequence of 3->2->1 (or 1->2->1) where the element 0 data is
>> unchanged
>> between 2->1, but was changed from 3->2 (or from 1->2), which may be observed by
>> the
>> static call.
> 
> 
> I'll agree that we need to add synchronization between 1->0->1, but you
> have not convinced me on this second part.

In addition to 1->0->1, there are actually 2 other scenarios here:

Transition 1->2 to which the ordering question between RCU and static call is
related.

Transition 2->1 would need an unconditional rcu-sync, because of transitions
3->2->1 and 1->2->1 which can lead the static call to observe wrong data if the
rcu_dereference happens when there are e.g. 3 callbacks registered, and then the
static call to the function (single callback) is called on the 3-callbacks array.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

  reply	other threads:[~2021-07-26 19:12 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-23  2:33 [PATCH] tracepoints: Update static_call before tp_funcs when adding a tracepoint Steven Rostedt
2021-07-26 15:46 ` Mathieu Desnoyers
2021-07-26 16:56   ` Steven Rostedt
2021-07-26 17:39     ` Mathieu Desnoyers
2021-07-26 18:49       ` Steven Rostedt
2021-07-26 19:12         ` Mathieu Desnoyers [this message]
2021-07-27 11:44         ` Peter Zijlstra
2021-07-27 13:46           ` Mathieu Desnoyers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=788674472.6819.1627326723120.JavaMail.zimbra@efficios.com \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox