| Message ID | 20081016094748.GD19019@ff.dom.local |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
Jarek Poplawski wrote: > pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation. > > + if (qops->peek == NULL) > + qops->peek = noop_qdisc_ops.peek; > if (qops->dequeue == NULL) > qops->dequeue = noop_qdisc_ops.dequeue; ->dequeue and ->peek are somewhat tied together, so I think we should only use the noop variants if both are unset. Whether this should be checked here of before merging new qdiscs is a different question of course :) -- 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 Thu, Oct 16, 2008 at 02:19:37PM +0200, Patrick McHardy wrote: > Jarek Poplawski wrote: >> pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation. >> > >> + if (qops->peek == NULL) >> + qops->peek = noop_qdisc_ops.peek; >> if (qops->dequeue == NULL) >> qops->dequeue = noop_qdisc_ops.dequeue; > > ->dequeue and ->peek are somewhat tied together, so I think we should > only use the noop variants if both are unset. Whether this should be > checked here of before merging new qdiscs is a different question of > course :) Actually, there is much less users of ->peek. Do you mean to always check for NULL before using? It was meant mainly for these non-work-conserving qdisc in case patch 6/6 isn't merged. Of course, IMHO it should be enough to implement this always (while merging), but this code above could be misleading what is optional/mandatory. (Please make it clear which way do you prefer and I'll redo, no problem.) 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
Jarek Poplawski wrote: > On Thu, Oct 16, 2008 at 02:19:37PM +0200, Patrick McHardy wrote: >> Jarek Poplawski wrote: >>> pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation. >>> >>> + if (qops->peek == NULL) >>> + qops->peek = noop_qdisc_ops.peek; >>> if (qops->dequeue == NULL) >>> qops->dequeue = noop_qdisc_ops.dequeue; >> ->dequeue and ->peek are somewhat tied together, so I think we should >> only use the noop variants if both are unset. Whether this should be >> checked here of before merging new qdiscs is a different question of >> course :) > > Actually, there is much less users of ->peek. Do you mean to always check > for NULL before using? It was meant mainly for these non-work-conserving > qdisc in case patch 6/6 isn't merged. Of course, IMHO it should be enough > to implement this always (while merging), but this code above could be > misleading what is optional/mandatory. (Please make it clear which way do > you prefer and I'll redo, no problem.) No, I meant that peek = noop_qdisc_ops.peek and dequeue = something_else doesn't make much sense. But I think declaring this an API usage error and catching it during review is fine. -- 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
Patrick McHardy wrote, On 10/16/2008 02:45 PM: > Jarek Poplawski wrote: >> On Thu, Oct 16, 2008 at 02:19:37PM +0200, Patrick McHardy wrote: >>> Jarek Poplawski wrote: >>>> pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation. >>>> >>>> + if (qops->peek == NULL) >>>> + qops->peek = noop_qdisc_ops.peek; >>>> if (qops->dequeue == NULL) >>>> qops->dequeue = noop_qdisc_ops.dequeue; >>> ->dequeue and ->peek are somewhat tied together, so I think we should >>> only use the noop variants if both are unset. Whether this should be >>> checked here of before merging new qdiscs is a different question of >>> course :) >> Actually, there is much less users of ->peek. Do you mean to always check >> for NULL before using? It was meant mainly for these non-work-conserving >> qdisc in case patch 6/6 isn't merged. Of course, IMHO it should be enough >> to implement this always (while merging), but this code above could be >> misleading what is optional/mandatory. (Please make it clear which way do >> you prefer and I'll redo, no problem.) > > No, I meant that peek = noop_qdisc_ops.peek and dequeue = something_else > doesn't make much sense. But I think declaring this an API usage error > and catching it during review is fine. As a matter of fact I treated this more symbolically: to declare the methods which don't need checking. Is it possible to forget about ->dequeue()? And after all, all qdisc but ingress declare this, so logically this could be removed. 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
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 1122c95..6cdc4e0 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -102,6 +102,10 @@ static int tclass_notify(struct sk_buff *oskb, struct nlmsghdr *n, requeues once dequeued packet. It is used for non-standard or just buggy devices, which can defer output even if netif_queue_stopped()=0. + ---peek + + like dequeue but without removing a packet from the queue + ---reset returns qdisc to initial state: purge all buffers, clear all @@ -149,6 +153,8 @@ int register_qdisc(struct Qdisc_ops *qops) qops->enqueue = noop_qdisc_ops.enqueue; if (qops->requeue == NULL) qops->requeue = noop_qdisc_ops.requeue; + if (qops->peek == NULL) + qops->peek = noop_qdisc_ops.peek; if (qops->dequeue == NULL) qops->dequeue = noop_qdisc_ops.dequeue; diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 7b5572d..ccf7e04 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -320,6 +320,7 @@ struct Qdisc_ops noop_qdisc_ops __read_mostly = { .priv_size = 0, .enqueue = noop_enqueue, .dequeue = noop_dequeue, + .peek = noop_dequeue, .requeue = noop_requeue, .owner = THIS_MODULE, }; @@ -345,6 +346,7 @@ static struct Qdisc_ops noqueue_qdisc_ops __read_mostly = { .priv_size = 0, .enqueue = noop_enqueue, .dequeue = noop_dequeue, + .peek = noop_dequeue, .requeue = noop_requeue, .owner = THIS_MODULE, }; @@ -409,6 +411,19 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc* qdisc) return NULL; } +static struct sk_buff *pfifo_fast_peek(struct Qdisc* qdisc) +{ + int prio; + struct sk_buff_head *list = qdisc_priv(qdisc); + + for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) { + if (!skb_queue_empty(list + prio)) + return skb_peek(list + prio); + } + + return NULL; +} + static int pfifo_fast_requeue(struct sk_buff *skb, struct Qdisc* qdisc) { qdisc->q.qlen++; @@ -455,6 +470,7 @@ static struct Qdisc_ops pfifo_fast_ops __read_mostly = { .priv_size = PFIFO_FAST_BANDS * sizeof(struct sk_buff_head), .enqueue = pfifo_fast_enqueue, .dequeue = pfifo_fast_dequeue, + .peek = pfifo_fast_peek, .requeue = pfifo_fast_requeue, .init = pfifo_fast_init, .reset = pfifo_fast_reset,
pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation. Signed-off-by: Jarek Poplawski <jarkao2@gmail.com> --- net/sched/sch_api.c | 6 ++++++ net/sched/sch_generic.c | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 0 deletions(-)