* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.