| Message ID | 20090821.170330.217935645.davem@davemloft.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 21 Aug 2009, David Miller wrote: > > None of this stuff should execute in hw IRQ context, therefore > use a tasklet_hrtimer so that it runs in softirq context. > > Signed-off-by: David S. Miller <davem@davemloft.net> Acked-by: Thomas Gleixner <tglx@linutronix.de> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote, On 08/22/2009 02:03 AM: > None of this stuff should execute in hw IRQ context, therefore > use a tasklet_hrtimer so that it runs in softirq context. Do you mean __netif_schedule()? Could you explain it more? Thanks, Jarek P. > > Signed-off-by: David S. Miller <davem@davemloft.net> > --- > include/net/pkt_sched.h | 4 ++-- > net/sched/sch_api.c | 8 ++++---- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h > index 82a3191..7eafb8d 100644 > --- a/include/net/pkt_sched.h > +++ b/include/net/pkt_sched.h > @@ -61,8 +61,8 @@ psched_tdiff_bounded(psched_time_t tv1, psched_time_t tv2, psched_time_t bound) > } > > struct qdisc_watchdog { > - struct hrtimer timer; > - struct Qdisc *qdisc; > + struct tasklet_hrtimer timer; > + struct Qdisc *qdisc; > }; > > extern void qdisc_watchdog_init(struct qdisc_watchdog *wd, struct Qdisc *qdisc); > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c > index 24d17ce..e1c2bf7 100644 > --- a/net/sched/sch_api.c > +++ b/net/sched/sch_api.c > @@ -468,8 +468,8 @@ static enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer) > > void qdisc_watchdog_init(struct qdisc_watchdog *wd, struct Qdisc *qdisc) > { > - hrtimer_init(&wd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > - wd->timer.function = qdisc_watchdog; > + tasklet_hrtimer_init(&wd->timer, qdisc_watchdog, > + CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > wd->qdisc = qdisc; > } > EXPORT_SYMBOL(qdisc_watchdog_init); > @@ -485,13 +485,13 @@ void qdisc_watchdog_schedule(struct qdisc_watchdog *wd, psched_time_t expires) > wd->qdisc->flags |= TCQ_F_THROTTLED; > time = ktime_set(0, 0); > time = ktime_add_ns(time, PSCHED_TICKS2NS(expires)); > - hrtimer_start(&wd->timer, time, HRTIMER_MODE_ABS); > + tasklet_hrtimer_start(&wd->timer, time, HRTIMER_MODE_ABS); > } > EXPORT_SYMBOL(qdisc_watchdog_schedule); > > void qdisc_watchdog_cancel(struct qdisc_watchdog *wd) > { > - hrtimer_cancel(&wd->timer); > + tasklet_hrtimer_cancel(&wd->timer); > wd->qdisc->flags &= ~TCQ_F_THROTTLED; > } > EXPORT_SYMBOL(qdisc_watchdog_cancel); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jarek Poplawski <jarkao2@gmail.com> Date: Sat, 22 Aug 2009 14:04:31 +0200 > David Miller wrote, On 08/22/2009 02:03 AM: > >> None of this stuff should execute in hw IRQ context, therefore >> use a tasklet_hrtimer so that it runs in softirq context. > > Do you mean __netif_schedule()? No, that works fine from HW irq context. > Could you explain it more? It's that throttled flag bit setting. It's still unsafe but at least we're in a position now to use softirq locking or atomic bitops to maintain it properly. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Thomas Gleixner <tglx@linutronix.de> Date: Sat, 22 Aug 2009 10:41:18 +0200 (CEST) > On Fri, 21 Aug 2009, David Miller wrote: > >> >> None of this stuff should execute in hw IRQ context, therefore >> use a tasklet_hrtimer so that it runs in softirq context. >> >> Signed-off-by: David S. Miller <davem@davemloft.net> > > Acked-by: Thomas Gleixner <tglx@linutronix.de> Thanks for reviewing. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22-08-2009 02:03, David Miller wrote: > None of this stuff should execute in hw IRQ context, therefore > use a tasklet_hrtimer so that it runs in softirq context. > > Signed-off-by: David S. Miller <davem@davemloft.net> > --- ... > void qdisc_watchdog_cancel(struct qdisc_watchdog *wd) > { > - hrtimer_cancel(&wd->timer); > + tasklet_hrtimer_cancel(&wd->timer); > wd->qdisc->flags &= ~TCQ_F_THROTTLED; > } > EXPORT_SYMBOL(qdisc_watchdog_cancel); I've had a second look at it and I wonder why it's not reported, but since qdisc_watchdog_cancel is run in qdisc_reset with BHs disabled it seems there should be (at least) a warning triggered in tasklet_kill on positive in_interrupt test (unless I miss something, but can't test it now). Jarek P. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jarek Poplawski <jarkao2@gmail.com> Date: Thu, 27 Aug 2009 08:36:41 +0000 > On 22-08-2009 02:03, David Miller wrote: >> None of this stuff should execute in hw IRQ context, therefore >> use a tasklet_hrtimer so that it runs in softirq context. >> >> Signed-off-by: David S. Miller <davem@davemloft.net> >> --- > ... >> void qdisc_watchdog_cancel(struct qdisc_watchdog *wd) >> { >> - hrtimer_cancel(&wd->timer); >> + tasklet_hrtimer_cancel(&wd->timer); >> wd->qdisc->flags &= ~TCQ_F_THROTTLED; >> } >> EXPORT_SYMBOL(qdisc_watchdog_cancel); > > I've had a second look at it and I wonder why it's not reported, but > since qdisc_watchdog_cancel is run in qdisc_reset with BHs disabled > it seems there should be (at least) a warning triggered in > tasklet_kill on positive in_interrupt test (unless I miss something, > but can't test it now). There are a lot of unknowns about this stuff. It's very likely I'm going to simply revert these two patches and try to work on these issues in net-next-2.6 instead. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index 82a3191..7eafb8d 100644 --- a/include/net/pkt_sched.h +++ b/include/net/pkt_sched.h @@ -61,8 +61,8 @@ psched_tdiff_bounded(psched_time_t tv1, psched_time_t tv2, psched_time_t bound) } struct qdisc_watchdog { - struct hrtimer timer; - struct Qdisc *qdisc; + struct tasklet_hrtimer timer; + struct Qdisc *qdisc; }; extern void qdisc_watchdog_init(struct qdisc_watchdog *wd, struct Qdisc *qdisc); diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 24d17ce..e1c2bf7 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -468,8 +468,8 @@ static enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer) void qdisc_watchdog_init(struct qdisc_watchdog *wd, struct Qdisc *qdisc) { - hrtimer_init(&wd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); - wd->timer.function = qdisc_watchdog; + tasklet_hrtimer_init(&wd->timer, qdisc_watchdog, + CLOCK_MONOTONIC, HRTIMER_MODE_ABS); wd->qdisc = qdisc; } EXPORT_SYMBOL(qdisc_watchdog_init); @@ -485,13 +485,13 @@ void qdisc_watchdog_schedule(struct qdisc_watchdog *wd, psched_time_t expires) wd->qdisc->flags |= TCQ_F_THROTTLED; time = ktime_set(0, 0); time = ktime_add_ns(time, PSCHED_TICKS2NS(expires)); - hrtimer_start(&wd->timer, time, HRTIMER_MODE_ABS); + tasklet_hrtimer_start(&wd->timer, time, HRTIMER_MODE_ABS); } EXPORT_SYMBOL(qdisc_watchdog_schedule); void qdisc_watchdog_cancel(struct qdisc_watchdog *wd) { - hrtimer_cancel(&wd->timer); + tasklet_hrtimer_cancel(&wd->timer); wd->qdisc->flags &= ~TCQ_F_THROTTLED; } EXPORT_SYMBOL(qdisc_watchdog_cancel);
None of this stuff should execute in hw IRQ context, therefore use a tasklet_hrtimer so that it runs in softirq context. Signed-off-by: David S. Miller <davem@davemloft.net> --- include/net/pkt_sched.h | 4 ++-- net/sched/sch_api.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)