Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1165628 > unrolled thread

Re: call_rcu from trace_preempt

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: call_rcu from trace_preempt Alexei Starovoitov <ast@plumgrid.com> - 2015-06-16 03:20 +0200
    Re: call_rcu from trace_preempt Alexei Starovoitov <ast@plumgrid.com> - 2015-06-16 19:30 +0200
      Re: call_rcu from trace_preempt Steven Rostedt <rostedt@goodmis.org> - 2015-06-16 19:40 +0200
      Re: call_rcu from trace_preempt "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-06-17 22:40 +0200
        Re: call_rcu from trace_preempt Alexei Starovoitov <ast@plumgrid.com> - 2015-06-17 23:00 +0200
          Re: call_rcu from trace_preempt "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-06-17 23:40 +0200
            Re: call_rcu from trace_preempt Alexei Starovoitov <ast@plumgrid.com> - 2015-06-18 02:00 +0200
              Re: call_rcu from trace_preempt "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-06-18 02:30 +0200
    Re: call_rcu from trace_preempt "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-06-16 19:50 +0200
      Re: call_rcu from trace_preempt Steven Rostedt <rostedt@goodmis.org> - 2015-06-16 21:00 +0200
        Re: call_rcu from trace_preempt "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-06-16 21:30 +0200
          Re: call_rcu from trace_preempt Steven Rostedt <rostedt@goodmis.org> - 2015-06-16 21:30 +0200
            Re: call_rcu from trace_preempt "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-06-16 21:40 +0200

#1165628 — Re: call_rcu from trace_preempt

On 6/15/15 4:07 PM, Paul E. McKenney wrote:
>
> Oh...  One important thing is that both call_rcu() and kfree_rcu()
> use per-CPU variables, managing a per-CPU linked list.  This is why
> they disable interrupts.  If you do another call_rcu() in the middle
> of the first one in just the wrong place, you will have two entities
> concurrently manipulating the same linked list, which will not go well.

yes. I'm trying to find that 'wrong place'.
The trace.patch is doing kmalloc/kfree_rcu for every preempt_enable.
So any spin_unlock called by first call_rcu will be triggering
2nd recursive to call_rcu.
But as far as I could understand rcu code that looks ok everywhere.
call_rcu
   debug_rcu_head_[un]queue
     debug_object_activate
       spin_unlock

and debug_rcu_head* seems to be called from safe places
where local_irq is enabled.

> Maybe mark call_rcu() and the things it calls as notrace?  Or you
> could maintain a separate per-CPU linked list that gathered up the
> stuff to be kfree()ed after a grace period, and some time later
> feed them to kfree_rcu()?

yeah, I can think of this or 10 other ways to fix it within
kprobe+bpf area, but I think something like call_rcu_notrace()
may be a better solution.
Or may be single generic 'fix' for call_rcu will be enough if
it doesn't affect all other users.

> The usual consequence of racing a pair of callback insertions on the
> same CPU would be that one of them gets leaked, and possible all
> subsequent callbacks.  So the lockup is no surprise.  And there are a
> lot of other assumptions in nearby code paths about only one execution
> at a time from a given CPU.

yes, I don't think calling 2nd call_rcu from preempt_enable violates
this assumptions. local_irq does it job. No extra stuff is called when
interrupts are disabled.

>> Any advise on where to look is greatly appreciated.
>
> What I don't understand is exactly what you are trying to do.  Have more
> complex tracers that dynamically allocate memory?  If so, having a per-CPU
> list that stages memory to be freed so that it can be passed to call_rcu()
> in a safe environment might make sense.  Of course, that list would need
> to be managed carefully!

yes. We tried to compute the time the kernel spends between
preempt_disable->preempt_enable and plot a histogram of latencies.

> Or am I missing the point of the code below?

this trace.patch is reproducer of call_rcu crashes that doing:
preempt_enable
   trace_preempt_on
     kfree_call_rcu

The real call stack is:
preempt_enable
   trace_preempt_on
     kprobe_int3_handler
       trace_call_bpf
         bpf_map_update_elem
           htab_map_update_elem
             kree_call_rcu

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1166278

On 6/16/15 5:38 AM, Daniel Wagner wrote:
> static int free_thread(void *arg)
> +{
> +	unsigned long flags;
> +	struct htab_elem *l;
> +
> +	while (!kthread_should_stop()) {
> +		spin_lock_irqsave(&elem_freelist_lock, flags);
> +		while (!list_empty(&elem_freelist)) {
> +			l = list_entry(elem_freelist.next,
> +				struct htab_elem, list);
> +			list_del(&l->list);
> +			kfree(l);

that's not right, since such thread defeats rcu protection of lookup.
We need either kfree_rcu/call_rcu or synchronize_rcu.
Obviously the former is preferred that's why I'm still digging into it.
Probably a thread that does kfree_rcu would be ok, but we shouldn't
be doing it unconditionally. For all networking programs and 99%
of tracing programs the existing code is fine and I don't want to
slow it down to tackle the corner case.
Extra spin_lock just to add it to the list is also quite costly.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1166288

On Tue, 16 Jun 2015 10:20:05 -0700
Alexei Starovoitov <ast@plumgrid.com> wrote:

> On 6/16/15 5:38 AM, Daniel Wagner wrote:
> > static int free_thread(void *arg)
> > +{
> > +	unsigned long flags;
> > +	struct htab_elem *l;
> > +
> > +	while (!kthread_should_stop()) {
> > +		spin_lock_irqsave(&elem_freelist_lock, flags);
> > +		while (!list_empty(&elem_freelist)) {
> > +			l = list_entry(elem_freelist.next,
> > +				struct htab_elem, list);
> > +			list_del(&l->list);
> > +			kfree(l);
> 
> that's not right, since such thread defeats rcu protection of lookup.
> We need either kfree_rcu/call_rcu or synchronize_rcu.
> Obviously the former is preferred that's why I'm still digging into it.
> Probably a thread that does kfree_rcu would be ok, but we shouldn't
> be doing it unconditionally. For all networking programs and 99%
> of tracing programs the existing code is fine and I don't want to
> slow it down to tackle the corner case.
> Extra spin_lock just to add it to the list is also quite costly.

Use a irq_work() handler to do the kfree_rcu(), and use llist (lockless
list) to add items to the list.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1167136

On Wed, Jun 17, 2015 at 11:39:29AM -0700, Alexei Starovoitov wrote:
> On 6/17/15 2:05 AM, Daniel Wagner wrote:
> >>>Steven's suggestion deferring the work via irq_work results in the same
> >>>stack trace. (Now I get cold feets, without the nice heat from the CPU
> >>>busy looping...)
> >That one still not working. It also makes the system really really slow.
> >I guess I still do something completely wrong.
> 
> tried your irq_work patch. It indeed makes the whole system
> unresponsive. Ctrl-C of hwlathist no longer works and
> it runs out of memory in 20 sec or so of running hwlathist
> on idle system (without parallel hackbench).
> It looks that free_pending flag is racy, so I removed it,
> but it didn't help.
> 
> Also I've tried all sort of other things in rcu including
> add rcu_bpf similar to rcu_sched to make sure that recursive
> call into call_rcu will not be messing rcu_preempt or rcu_sched
> states and instead will be operating on rcu_bpf per-cpu states.
> In theory that should have worked flawlessly and it sort-of did.
> But multiple hackbench runs still managed to crash it.
> So far I think the temp workaround is to stick with array maps
> for probing such low level things like trace_preempt.
> Note that pre-allocation of all elements in hash map also won't
> help, since the problem here is some collision of call_rcu and
> rcu_process_callbacks. I'm pretty sure that kfree_rcu with
> rcu_is_watching patch is ready for this type of abuse.
> The rcu_process_callbacks() path - no yet. I'm still analyzing it.

How about if I just gave you a hook in __call_rcu() itself, just before
it returns, just after the local_irq_restore()?  You could maintain
recursion flags and test the environment, at some point handling any
memory that needed freeing.

The idea would be to use an atomic queue to accumulate the to-be-freed
data, then kfree_rcu() it in the hook if it was safe to do so.

I really don't trust the re-entrancy, especially not in the long term.

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1167139

On 6/17/15 1:37 PM, Paul E. McKenney wrote:
> On Wed, Jun 17, 2015 at 11:39:29AM -0700, Alexei Starovoitov wrote:
>> On 6/17/15 2:05 AM, Daniel Wagner wrote:
>>>>> Steven's suggestion deferring the work via irq_work results in the same
>>>>> stack trace. (Now I get cold feets, without the nice heat from the CPU
>>>>> busy looping...)
>>> That one still not working. It also makes the system really really slow.
>>> I guess I still do something completely wrong.
>>
>> tried your irq_work patch. It indeed makes the whole system
>> unresponsive. Ctrl-C of hwlathist no longer works and
>> it runs out of memory in 20 sec or so of running hwlathist
>> on idle system (without parallel hackbench).
>> It looks that free_pending flag is racy, so I removed it,
>> but it didn't help.
>>
>> Also I've tried all sort of other things in rcu including
>> add rcu_bpf similar to rcu_sched to make sure that recursive
>> call into call_rcu will not be messing rcu_preempt or rcu_sched
>> states and instead will be operating on rcu_bpf per-cpu states.
>> In theory that should have worked flawlessly and it sort-of did.
>> But multiple hackbench runs still managed to crash it.
>> So far I think the temp workaround is to stick with array maps
>> for probing such low level things like trace_preempt.
>> Note that pre-allocation of all elements in hash map also won't
>> help, since the problem here is some collision of call_rcu and
>> rcu_process_callbacks. I'm pretty sure that kfree_rcu with
>> rcu_is_watching patch is ready for this type of abuse.
>> The rcu_process_callbacks() path - no yet. I'm still analyzing it.
>
> How about if I just gave you a hook in __call_rcu() itself, just before
> it returns, just after the local_irq_restore()?  You could maintain
> recursion flags and test the environment, at some point handling any
> memory that needed freeing.
>
> The idea would be to use an atomic queue to accumulate the to-be-freed
> data, then kfree_rcu() it in the hook if it was safe to do so.

I'm not yet seeing how it will look. You mean I'll just locklessly
enqueue into some global llist and it will get kfree-d from
rcu_process_callbacks() ?
Something like kfree_rcu_lockless(ptr_to_be_freed) that
llist_adds and let rcu core know that something has to be freed?
I think such feature would be very useful in general.
Or may be kfree_rcu_this_cpu(ptr_to_be_freed) that uses
per-cpu llist of 'to-be-kfreed' objects?
Performance will be great and not need to embed rcu_head in
every datastructure.

btw, irq_work suffers the same re-entrancy problem:
[   19.914910]  [<ffffffff8117c63c>] free_work_cb+0x2c/0x50
[   19.914910]  [<ffffffff81176624>] irq_work_run_list+0x44/0x70
[   19.914910]  [<ffffffff8117667e>] irq_work_run+0x2e/0x50
[   19.914910]  [<ffffffff81008d0e>] smp_irq_work_interrupt+0x2e/0x40
[   19.914910]  [<ffffffff8178dd20>] irq_work_interrupt+0x70/0x80
[   19.914910]  <EOI>  [<ffffffff813d3cec>] ? 
debug_object_active_state+0xfc/0x150
[   19.914910]  [<ffffffff813d3cec>] ? debug_object_active_state+0xfc/0x150
[   19.914910]  [<ffffffff8178c21b>] ? _raw_spin_unlock_irqrestore+0x4b/0x80
[   19.914910]  [<ffffffff8115e0a7>] ? trace_preempt_on+0x7/0x100
[   19.914910]  [<ffffffff810991c3>] ? preempt_count_sub+0x73/0xf0
[   19.914910]  [<ffffffff8178c21b>] _raw_spin_unlock_irqrestore+0x4b/0x80
[   19.914910]  [<ffffffff813d3cec>] debug_object_active_state+0xfc/0x150
[   19.914910]  [<ffffffff812035f0>] ? get_max_files+0x20/0x20
[   19.914910]  [<ffffffff810e2d2f>] __call_rcu.constprop.67+0x5f/0x350
[   19.914910]  [<ffffffff810e3097>] call_rcu+0x17/0x20
[   19.914910]  [<ffffffff81203843>] __fput+0x183/0x200

so if I do call_rcu from free_work_cb, it's equally bad
as calling call_rcu from trace_preempt_on


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1167163

On Wed, Jun 17, 2015 at 01:53:17PM -0700, Alexei Starovoitov wrote:
> On 6/17/15 1:37 PM, Paul E. McKenney wrote:
> >On Wed, Jun 17, 2015 at 11:39:29AM -0700, Alexei Starovoitov wrote:
> >>On 6/17/15 2:05 AM, Daniel Wagner wrote:
> >>>>>Steven's suggestion deferring the work via irq_work results in the same
> >>>>>stack trace. (Now I get cold feets, without the nice heat from the CPU
> >>>>>busy looping...)
> >>>That one still not working. It also makes the system really really slow.
> >>>I guess I still do something completely wrong.
> >>
> >>tried your irq_work patch. It indeed makes the whole system
> >>unresponsive. Ctrl-C of hwlathist no longer works and
> >>it runs out of memory in 20 sec or so of running hwlathist
> >>on idle system (without parallel hackbench).
> >>It looks that free_pending flag is racy, so I removed it,
> >>but it didn't help.
> >>
> >>Also I've tried all sort of other things in rcu including
> >>add rcu_bpf similar to rcu_sched to make sure that recursive
> >>call into call_rcu will not be messing rcu_preempt or rcu_sched
> >>states and instead will be operating on rcu_bpf per-cpu states.
> >>In theory that should have worked flawlessly and it sort-of did.
> >>But multiple hackbench runs still managed to crash it.
> >>So far I think the temp workaround is to stick with array maps
> >>for probing such low level things like trace_preempt.
> >>Note that pre-allocation of all elements in hash map also won't
> >>help, since the problem here is some collision of call_rcu and
> >>rcu_process_callbacks. I'm pretty sure that kfree_rcu with
> >>rcu_is_watching patch is ready for this type of abuse.
> >>The rcu_process_callbacks() path - no yet. I'm still analyzing it.
> >
> >How about if I just gave you a hook in __call_rcu() itself, just before
> >it returns, just after the local_irq_restore()?  You could maintain
> >recursion flags and test the environment, at some point handling any
> >memory that needed freeing.
> >
> >The idea would be to use an atomic queue to accumulate the to-be-freed
> >data, then kfree_rcu() it in the hook if it was safe to do so.
> 
> I'm not yet seeing how it will look. You mean I'll just locklessly
> enqueue into some global llist and it will get kfree-d from
> rcu_process_callbacks() ?

Locklessly enqueue onto a per-CPU list, but yes.  The freeing is up to
you -- you get called just before exit from __call_rcu(), and get to
figure out what to do.

My guess would be if not in interrupt and not recursively invoked,
atomically remove all the elements from the list, then pass each to
kfree_rcu(), and finally let things take their course from there.
The llist APIs look like they would work.

> Something like kfree_rcu_lockless(ptr_to_be_freed) that
> llist_adds and let rcu core know that something has to be freed?
> I think such feature would be very useful in general.
> Or may be kfree_rcu_this_cpu(ptr_to_be_freed) that uses
> per-cpu llist of 'to-be-kfreed' objects?
> Performance will be great and not need to embed rcu_head in
> every datastructure.

Well, you do need to have something in each element to allow them to be
tracked.  You could indeed use llist_add() to maintain the per-CPU list,
and then use llist_del_all() bulk-remove all the elements from the per-CPU
list.  You can then pass each element in turn to kfree_rcu().  And yes,
I am suggesting that you open-code this, as it is going to be easier to
handle your special case then to provide a fully general solution.  For
one thing, the general solution would require a full rcu_head to track
offset and next.  In contrast, you can special-case the offset.  And
ignore the overload special cases.

							Thanx, Paul

> btw, irq_work suffers the same re-entrancy problem:
> [   19.914910]  [<ffffffff8117c63c>] free_work_cb+0x2c/0x50
> [   19.914910]  [<ffffffff81176624>] irq_work_run_list+0x44/0x70
> [   19.914910]  [<ffffffff8117667e>] irq_work_run+0x2e/0x50
> [   19.914910]  [<ffffffff81008d0e>] smp_irq_work_interrupt+0x2e/0x40
> [   19.914910]  [<ffffffff8178dd20>] irq_work_interrupt+0x70/0x80
> [   19.914910]  <EOI>  [<ffffffff813d3cec>] ?
> debug_object_active_state+0xfc/0x150
> [   19.914910]  [<ffffffff813d3cec>] ? debug_object_active_state+0xfc/0x150
> [   19.914910]  [<ffffffff8178c21b>] ? _raw_spin_unlock_irqrestore+0x4b/0x80
> [   19.914910]  [<ffffffff8115e0a7>] ? trace_preempt_on+0x7/0x100
> [   19.914910]  [<ffffffff810991c3>] ? preempt_count_sub+0x73/0xf0
> [   19.914910]  [<ffffffff8178c21b>] _raw_spin_unlock_irqrestore+0x4b/0x80
> [   19.914910]  [<ffffffff813d3cec>] debug_object_active_state+0xfc/0x150
> [   19.914910]  [<ffffffff812035f0>] ? get_max_files+0x20/0x20
> [   19.914910]  [<ffffffff810e2d2f>] __call_rcu.constprop.67+0x5f/0x350
> [   19.914910]  [<ffffffff810e3097>] call_rcu+0x17/0x20
> [   19.914910]  [<ffffffff81203843>] __fput+0x183/0x200
> 
> so if I do call_rcu from free_work_cb, it's equally bad
> as calling call_rcu from trace_preempt_on
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1167493

On 6/17/15 2:36 PM, Paul E. McKenney wrote:
> Well, you do need to have something in each element to allow them to be
> tracked.  You could indeed use llist_add() to maintain the per-CPU list,
> and then use llist_del_all() bulk-remove all the elements from the per-CPU
> list.  You can then pass each element in turn to kfree_rcu().  And yes,
> I am suggesting that you open-code this, as it is going to be easier to
> handle your special case then to provide a fully general solution.  For
> one thing, the general solution would require a full rcu_head to track
> offset and next.  In contrast, you can special-case the offset.  And
> ignore the overload special cases.

yes. all makes sense.

 > Locklessly enqueue onto a per-CPU list, but yes.  The freeing is up to

yes. per-cpu llist indeed.

 > you -- you get called just before exit from __call_rcu(), and get to
 > figure out what to do.
 >
 > My guess would be if not in interrupt and not recursively invoked,
 > atomically remove all the elements from the list, then pass each to
 > kfree_rcu(), and finally let things take their course from there.
 > The llist APIs look like they would work.

Above and 'just before the exit from __call_rcu()' part of suggestion
I still don't understand.
To avoid reentry into call_rcu I can either create 1 or N new kthreads
or work_queue and do manual wakeups, but that's very specialized and I
don't want to permanently waste them, so I'm thinking to llist_add into
per-cpu llists and do llist_del_all in rcu_process_callbacks() to take
them from these llists and call kfree_rcu on them.
The llist_add part will also do:
if (!rcu_is_watching()) invoke_rcu_core();
to raise softirq when necessary.
So at the end it will look like two phase kfree_rcu.
I'll try to code it up and see it explodes :)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1167508

On Wed, Jun 17, 2015 at 04:58:48PM -0700, Alexei Starovoitov wrote:
> On 6/17/15 2:36 PM, Paul E. McKenney wrote:
> >Well, you do need to have something in each element to allow them to be
> >tracked.  You could indeed use llist_add() to maintain the per-CPU list,
> >and then use llist_del_all() bulk-remove all the elements from the per-CPU
> >list.  You can then pass each element in turn to kfree_rcu().  And yes,
> >I am suggesting that you open-code this, as it is going to be easier to
> >handle your special case then to provide a fully general solution.  For
> >one thing, the general solution would require a full rcu_head to track
> >offset and next.  In contrast, you can special-case the offset.  And
> >ignore the overload special cases.
> 
> yes. all makes sense.
> 
> > Locklessly enqueue onto a per-CPU list, but yes.  The freeing is up to
> 
> yes. per-cpu llist indeed.
> 
> > you -- you get called just before exit from __call_rcu(), and get to
> > figure out what to do.
> >
> > My guess would be if not in interrupt and not recursively invoked,
> > atomically remove all the elements from the list, then pass each to
> > kfree_rcu(), and finally let things take their course from there.
> > The llist APIs look like they would work.
> 
> Above and 'just before the exit from __call_rcu()' part of suggestion
> I still don't understand.
> To avoid reentry into call_rcu I can either create 1 or N new kthreads
> or work_queue and do manual wakeups, but that's very specialized and I
> don't want to permanently waste them, so I'm thinking to llist_add into
> per-cpu llists and do llist_del_all in rcu_process_callbacks() to take
> them from these llists and call kfree_rcu on them.

Another option is to drain the lists the next time you do an allocation.
That would avoid hooking both __call_rcu() and rcu_process_callbacks().

							Thanx, Paul

> The llist_add part will also do:
> if (!rcu_is_watching()) invoke_rcu_core();
> to raise softirq when necessary.
> So at the end it will look like two phase kfree_rcu.
> I'll try to code it up and see it explodes :)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1166293

On Tue, Jun 16, 2015 at 10:14:08AM -0700, Alexei Starovoitov wrote:
> On 6/16/15 9:05 AM, Paul E. McKenney wrote:
> >On Tue, Jun 16, 2015 at 11:37:38AM -0400, Steven Rostedt wrote:
> >>On Tue, 16 Jun 2015 05:27:33 -0700
> >>"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >>
> >>>On Mon, Jun 15, 2015 at 10:45:05PM -0700, Alexei Starovoitov wrote:
> >>>>On 6/15/15 7:14 PM, Paul E. McKenney wrote:
> >>>>>
> >>>>>Why do you believe that it is better to fix it within call_rcu()?
> >>>>
> >>>>found it:
> >>>>diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> >>>>index 8cf7304b2867..a3be09d482ae 100644
> >>>>--- a/kernel/rcu/tree.c
> >>>>+++ b/kernel/rcu/tree.c
> >>>>@@ -935,9 +935,9 @@ bool notrace rcu_is_watching(void)
> >>>>  {
> >>>>         bool ret;
> >>>>
> >>>>-       preempt_disable();
> >>>>+       preempt_disable_notrace();
> >>>>         ret = __rcu_is_watching();
> >>>>-       preempt_enable();
> >>>>+       preempt_enable_notrace();
> >>>>         return ret;
> >>>>  }
> >>>>
> >>>>the rcu_is_watching() and __rcu_is_watching() are already marked
> >>>>notrace, so imo it's a good 'fix'.
> >>>>What was happening is that the above preempt_enable was triggering
> >>>>recursive call_rcu that was indeed messing 'rdp' that was
> >>>>prepared by __call_rcu and before __call_rcu_core could use that.
> >>>
> >>>>btw, also noticed that local_irq_save done by note_gp_changes
> >>>>is partially redundant. In __call_rcu_core path the irqs are
> >>>>already disabled.
> >>>
> >>
> >>If rcu_is_watching() and __rcu_is_watching() are both marked as
> >>notrace, it makes sense to use preempt_disable/enable_notrace() as it
> >>otherwise defeats the purpose of the notrace markers on rcu_is_watching.

And __rcu_is_watching() is marked notrace as well.

> >>That is regardless of what the rest of this thread is about.
> >
> >Good enough!  Alexei, are you OK with my adding your Signed-off-by
> >to the above patch?
> 
> sure.
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> 
> >(Still not sold on reentrant call_rcu() and
> >kfree_rcu(), but getting notrace set up correctly is worthwhile.)
> 
> I'm not sold on it either. So far trying to understand
> all consequences.

Here is the updated patch.  Steven, I added your "Acked-by" based
on your positive comments above, please let me know if you would
like me to remove it.

							Thanx, Paul

------------------------------------------------------------------------

commit 9611f225d383a2edbdf74ca7f00c8d0b1e56dc45
Author: Alexei Starovoitov <ast@plumgrid.com>
Date:   Tue Jun 16 10:35:18 2015 -0700

    rcu: Make rcu_is_watching() really notrace
    
    Although rcu_is_watching() is marked notrace, it invokes preempt_disable()
    and preempt_enable(), both of which can be traced.  This defeats the
    purpose of the notrace on rcu_is_watching(), so this commit substitutes
    preempt_disable_notrace() and preempt_enable_notrace().
    
    Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Acked-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index fc0385380e97..c844ef3c2fae 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -973,9 +973,9 @@ bool notrace rcu_is_watching(void)
 {
 	bool ret;
 
-	preempt_disable();
+	preempt_disable_notrace();
 	ret = __rcu_is_watching();
-	preempt_enable();
+	preempt_enable_notrace();
 	return ret;
 }
 EXPORT_SYMBOL_GPL(rcu_is_watching);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1166319

On Tue, 16 Jun 2015 10:39:42 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> > >>If rcu_is_watching() and __rcu_is_watching() are both marked as
> > >>notrace, it makes sense to use preempt_disable/enable_notrace() as it
> > >>otherwise defeats the purpose of the notrace markers on rcu_is_watching.
> 
> And __rcu_is_watching() is marked notrace as well.

Isn't that what I said?

> 
> > >>That is regardless of what the rest of this thread is about.
> > >
> > >Good enough!  Alexei, are you OK with my adding your Signed-off-by
> > >to the above patch?
> > 
> > sure.
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > 
> > >(Still not sold on reentrant call_rcu() and
> > >kfree_rcu(), but getting notrace set up correctly is worthwhile.)
> > 
> > I'm not sold on it either. So far trying to understand
> > all consequences.
> 
> Here is the updated patch.  Steven, I added your "Acked-by" based
> on your positive comments above, please let me know if you would
> like me to remove it.

I'm fine with it. But doesn't Acked-by go above Signed-off-by?

-- Steve

> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit 9611f225d383a2edbdf74ca7f00c8d0b1e56dc45
> Author: Alexei Starovoitov <ast@plumgrid.com>
> Date:   Tue Jun 16 10:35:18 2015 -0700
> 
>     rcu: Make rcu_is_watching() really notrace
>     
>     Although rcu_is_watching() is marked notrace, it invokes preempt_disable()
>     and preempt_enable(), both of which can be traced.  This defeats the
>     purpose of the notrace on rcu_is_watching(), so this commit substitutes
>     preempt_disable_notrace() and preempt_enable_notrace().
>     
>     Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>     Acked-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index fc0385380e97..c844ef3c2fae 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -973,9 +973,9 @@ bool notrace rcu_is_watching(void)
>  {
>  	bool ret;
>  
> -	preempt_disable();
> +	preempt_disable_notrace();
>  	ret = __rcu_is_watching();
> -	preempt_enable();
> +	preempt_enable_notrace();
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(rcu_is_watching);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1166339

On Tue, Jun 16, 2015 at 02:57:44PM -0400, Steven Rostedt wrote:
> On Tue, 16 Jun 2015 10:39:42 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > > >>If rcu_is_watching() and __rcu_is_watching() are both marked as
> > > >>notrace, it makes sense to use preempt_disable/enable_notrace() as it
> > > >>otherwise defeats the purpose of the notrace markers on rcu_is_watching.
> > 
> > And __rcu_is_watching() is marked notrace as well.
> 
> Isn't that what I said?

You did say "if", so I checked.  ;-)

> > > >>That is regardless of what the rest of this thread is about.
> > > >
> > > >Good enough!  Alexei, are you OK with my adding your Signed-off-by
> > > >to the above patch?
> > > 
> > > sure.
> > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > > 
> > > >(Still not sold on reentrant call_rcu() and
> > > >kfree_rcu(), but getting notrace set up correctly is worthwhile.)
> > > 
> > > I'm not sold on it either. So far trying to understand
> > > all consequences.
> > 
> > Here is the updated patch.  Steven, I added your "Acked-by" based
> > on your positive comments above, please let me know if you would
> > like me to remove it.
> 
> I'm fine with it. But doesn't Acked-by go above Signed-off-by?

I have done it both ways, usually in time order.

							Thanx, Paul

> -- Steve
> 
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit 9611f225d383a2edbdf74ca7f00c8d0b1e56dc45
> > Author: Alexei Starovoitov <ast@plumgrid.com>
> > Date:   Tue Jun 16 10:35:18 2015 -0700
> > 
> >     rcu: Make rcu_is_watching() really notrace
> >     
> >     Although rcu_is_watching() is marked notrace, it invokes preempt_disable()
> >     and preempt_enable(), both of which can be traced.  This defeats the
> >     purpose of the notrace on rcu_is_watching(), so this commit substitutes
> >     preempt_disable_notrace() and preempt_enable_notrace().
> >     
> >     Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >     Acked-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index fc0385380e97..c844ef3c2fae 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -973,9 +973,9 @@ bool notrace rcu_is_watching(void)
> >  {
> >  	bool ret;
> >  
> > -	preempt_disable();
> > +	preempt_disable_notrace();
> >  	ret = __rcu_is_watching();
> > -	preempt_enable();
> > +	preempt_enable_notrace();
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(rcu_is_watching);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1166341

On Tue, 16 Jun 2015 12:20:55 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> > I'm fine with it. But doesn't Acked-by go above Signed-off-by?
> 
> I have done it both ways, usually in time order.

You're either a Big-endian, or a Little-endian? You can't be both!

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1166343

On Tue, Jun 16, 2015 at 03:29:25PM -0400, Steven Rostedt wrote:
> On Tue, 16 Jun 2015 12:20:55 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > > I'm fine with it. But doesn't Acked-by go above Signed-off-by?
> > 
> > I have done it both ways, usually in time order.
> 
> You're either a Big-endian, or a Little-endian? You can't be both!

We specify at boot time, on each boot.  Can be different for different
guest OSes.  ;-)

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web