* [2.6.32-rc5-git5] synchronize_sched() inside spin_lock()?
@ 2009-11-02 12:00 Tetsuo Handa
2009-11-02 22:30 ` Paul E. McKenney
2009-11-22 13:49 ` [2.6.32-rc8] set_all_monitor_traces() sleeping bug Tetsuo Handa
0 siblings, 2 replies; 5+ messages in thread
From: Tetsuo Handa @ 2009-11-02 12:00 UTC (permalink / raw)
To: nhorman; +Cc: linux-kernel
Commit: 4ea7e38696c7e798c47ebbecadfd392f23f814f9
tracepoint_synchronize_unregister() calls synchronize_sched(), but it is
between spin_lock() and spin_unlock(). Is it OK?
static int set_all_monitor_traces(int state)
{
int rc = 0;
struct dm_hw_stat_delta *new_stat = NULL;
struct dm_hw_stat_delta *temp;
spin_lock(&trace_state_lock);
switch (state) {
case TRACE_ON:
rc |= register_trace_kfree_skb(trace_kfree_skb_hit);
rc |= register_trace_napi_poll(trace_napi_poll_hit);
break;
case TRACE_OFF:
rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit);
rc |= unregister_trace_napi_poll(trace_napi_poll_hit);
tracepoint_synchronize_unregister();
/*
* Clean the device list
*/
list_for_each_entry_safe(new_stat, temp, &hw_stats_list, list) {
if (new_stat->dev == NULL) {
list_del_rcu(&new_stat->list);
call_rcu(&new_stat->rcu, free_dm_hw_stat);
}
}
break;
default:
rc = 1;
break;
}
if (!rc)
trace_state = state;
spin_unlock(&trace_state_lock);
if (rc)
return -EINPROGRESS;
return rc;
}
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [2.6.32-rc5-git5] synchronize_sched() inside spin_lock()?
2009-11-02 12:00 [2.6.32-rc5-git5] synchronize_sched() inside spin_lock()? Tetsuo Handa
@ 2009-11-02 22:30 ` Paul E. McKenney
2009-11-02 23:33 ` Dmitry Torokhov
2009-11-22 13:49 ` [2.6.32-rc8] set_all_monitor_traces() sleeping bug Tetsuo Handa
1 sibling, 1 reply; 5+ messages in thread
From: Paul E. McKenney @ 2009-11-02 22:30 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: nhorman, linux-kernel
On Mon, Nov 02, 2009 at 09:00:06PM +0900, Tetsuo Handa wrote:
> Commit: 4ea7e38696c7e798c47ebbecadfd392f23f814f9
>
> tracepoint_synchronize_unregister() calls synchronize_sched(), but it is
> between spin_lock() and spin_unlock(). Is it OK?
Calling synchronize_sched() while holding a spinlock would indeed be
very bad, but the code below seems to instead be invoking call_rcu(),
which is no problem.
Or am I missing something here?
Thanx, Paul
> static int set_all_monitor_traces(int state)
> {
> int rc = 0;
> struct dm_hw_stat_delta *new_stat = NULL;
> struct dm_hw_stat_delta *temp;
>
> spin_lock(&trace_state_lock);
>
> switch (state) {
> case TRACE_ON:
> rc |= register_trace_kfree_skb(trace_kfree_skb_hit);
> rc |= register_trace_napi_poll(trace_napi_poll_hit);
> break;
> case TRACE_OFF:
> rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit);
> rc |= unregister_trace_napi_poll(trace_napi_poll_hit);
>
> tracepoint_synchronize_unregister();
>
> /*
> * Clean the device list
> */
> list_for_each_entry_safe(new_stat, temp, &hw_stats_list, list) {
> if (new_stat->dev == NULL) {
> list_del_rcu(&new_stat->list);
> call_rcu(&new_stat->rcu, free_dm_hw_stat);
> }
> }
> break;
> default:
> rc = 1;
> break;
> }
>
> if (!rc)
> trace_state = state;
>
> spin_unlock(&trace_state_lock);
>
> if (rc)
> return -EINPROGRESS;
> return rc;
> }
> --
> 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/
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [2.6.32-rc5-git5] synchronize_sched() inside spin_lock()?
2009-11-02 22:30 ` Paul E. McKenney
@ 2009-11-02 23:33 ` Dmitry Torokhov
2009-11-03 0:55 ` Paul E. McKenney
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Torokhov @ 2009-11-02 23:33 UTC (permalink / raw)
To: paulmck; +Cc: Tetsuo Handa, nhorman, linux-kernel
On Monday 02 November 2009 02:30:28 pm Paul E. McKenney wrote:
> On Mon, Nov 02, 2009 at 09:00:06PM +0900, Tetsuo Handa wrote:
> > Commit: 4ea7e38696c7e798c47ebbecadfd392f23f814f9
> >
> > tracepoint_synchronize_unregister() calls synchronize_sched(), but it is
> > between spin_lock() and spin_unlock(). Is it OK?
>
> Calling synchronize_sched() while holding a spinlock would indeed be
> very bad, but the code below seems to instead be invoking call_rcu(),
> which is no problem.
>
> Or am I missing something here?
>
> Thanx, Paul
>
> > static int set_all_monitor_traces(int state)
> > {
> > int rc = 0;
> > struct dm_hw_stat_delta *new_stat = NULL;
> > struct dm_hw_stat_delta *temp;
> >
> > spin_lock(&trace_state_lock);
> >
> > switch (state) {
> > case TRACE_ON:
> > rc |= register_trace_kfree_skb(trace_kfree_skb_hit);
> > rc |= register_trace_napi_poll(trace_napi_poll_hit);
> > break;
> > case TRACE_OFF:
> > rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit);
> > rc |= unregister_trace_napi_poll(trace_napi_poll_hit);
> >
> > tracepoint_synchronize_unregister();
This has synchronize_sched() inside.
--
Dmitry
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [2.6.32-rc5-git5] synchronize_sched() inside spin_lock()?
2009-11-02 23:33 ` Dmitry Torokhov
@ 2009-11-03 0:55 ` Paul E. McKenney
0 siblings, 0 replies; 5+ messages in thread
From: Paul E. McKenney @ 2009-11-03 0:55 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Tetsuo Handa, nhorman, linux-kernel
On Mon, Nov 02, 2009 at 03:33:46PM -0800, Dmitry Torokhov wrote:
> On Monday 02 November 2009 02:30:28 pm Paul E. McKenney wrote:
> > On Mon, Nov 02, 2009 at 09:00:06PM +0900, Tetsuo Handa wrote:
> > > Commit: 4ea7e38696c7e798c47ebbecadfd392f23f814f9
> > >
> > > tracepoint_synchronize_unregister() calls synchronize_sched(), but it is
> > > between spin_lock() and spin_unlock(). Is it OK?
> >
> > Calling synchronize_sched() while holding a spinlock would indeed be
> > very bad, but the code below seems to instead be invoking call_rcu(),
> > which is no problem.
> >
> > Or am I missing something here?
> >
> > Thanx, Paul
> >
> > > static int set_all_monitor_traces(int state)
> > > {
> > > int rc = 0;
> > > struct dm_hw_stat_delta *new_stat = NULL;
> > > struct dm_hw_stat_delta *temp;
> > >
> > > spin_lock(&trace_state_lock);
> > >
> > > switch (state) {
> > > case TRACE_ON:
> > > rc |= register_trace_kfree_skb(trace_kfree_skb_hit);
> > > rc |= register_trace_napi_poll(trace_napi_poll_hit);
> > > break;
> > > case TRACE_OFF:
> > > rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit);
> > > rc |= unregister_trace_napi_poll(trace_napi_poll_hit);
> > >
> > > tracepoint_synchronize_unregister();
>
> This has synchronize_sched() inside.
Ah! That would indeed be bad.
Thanx, Paul
^ permalink raw reply [flat|nested] 5+ messages in thread
* [2.6.32-rc8] set_all_monitor_traces() sleeping bug.
2009-11-02 12:00 [2.6.32-rc5-git5] synchronize_sched() inside spin_lock()? Tetsuo Handa
2009-11-02 22:30 ` Paul E. McKenney
@ 2009-11-22 13:49 ` Tetsuo Handa
1 sibling, 0 replies; 5+ messages in thread
From: Tetsuo Handa @ 2009-11-22 13:49 UTC (permalink / raw)
To: nhorman, davem; +Cc: netdev, linux-kernel
Commit: 4ea7e38696c7e798c47ebbecadfd392f23f814f9
set_all_monitor_traces() calls tracepoint_synchronize_unregister()
with a spinlock held, but tracepoint_synchronize_unregister() may sleep.
static int set_all_monitor_traces(int state)
{
int rc = 0;
struct dm_hw_stat_delta *new_stat = NULL;
struct dm_hw_stat_delta *temp;
spin_lock(&trace_state_lock);
switch (state) {
case TRACE_ON:
rc |= register_trace_kfree_skb(trace_kfree_skb_hit);
rc |= register_trace_napi_poll(trace_napi_poll_hit);
break;
case TRACE_OFF:
rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit);
rc |= unregister_trace_napi_poll(trace_napi_poll_hit);
tracepoint_synchronize_unregister();
/*
* Clean the device list
*/
list_for_each_entry_safe(new_stat, temp, &hw_stats_list, list) {
if (new_stat->dev == NULL) {
list_del_rcu(&new_stat->list);
call_rcu(&new_stat->rcu, free_dm_hw_stat);
}
}
break;
default:
rc = 1;
break;
}
if (!rc)
trace_state = state;
spin_unlock(&trace_state_lock);
if (rc)
return -EINPROGRESS;
return rc;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-11-22 13:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-11-02 12:00 [2.6.32-rc5-git5] synchronize_sched() inside spin_lock()? Tetsuo Handa
2009-11-02 22:30 ` Paul E. McKenney
2009-11-02 23:33 ` Dmitry Torokhov
2009-11-03 0:55 ` Paul E. McKenney
2009-11-22 13:49 ` [2.6.32-rc8] set_all_monitor_traces() sleeping bug Tetsuo Handa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®