[PATCH 3/6] pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation.

[PATCH 3/6] pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation.

[Jarek Poplawski]

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(-)

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,
-- 
1.5.6.5

Re: [PATCH 3/6] pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation.

[Patrick McHardy]

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 :)

Re: [PATCH 3/6] pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation.

[Jarek Poplawski]

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.

Re: [PATCH 3/6] pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation.

[Patrick McHardy]

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.

Re: [PATCH 3/6] pkt_sched: sch_generic: Add generic qdisc->ops->peek() implementation.

[Jarek Poplawski]

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.