* [PATCH 00/14]: Killing qdisc->ops->requeue().
@ 2008-10-14 9:52 Jarek Poplawski
2008-10-14 11:39 ` Patrick McHardy
2008-10-14 16:41 ` Alexander Duyck
0 siblings, 2 replies; 16+ messages in thread
From: Jarek Poplawski @ 2008-10-14 9:52 UTC (permalink / raw)
To: David Miller; +Cc: netdev
The aim of this patch-set is to finish changes proposed by David S.
Miller in his patch-set with the same subject from Mon, 18 Aug 2008.
The first two patches were applied with some modifications, so, to
apply the rest, there were needed some changes.
Original David's patches include additional info, but signed-off-by
is removed because of changed context. I expect they will be merged
and signed off by David as an author, anyway.
The qdisc->requeue list idea is to limit requeuing to one level only,
so a parent can requeue to its child only. This list is then tried
first while dequeuing (qdisc_dequeue()), except at the top level,
so packets could be requeued only by qdiscs, not by qdisc_restart()
after xmit errors.
Thanks,
Jarek P.
include/net/sch_generic.h | 23 +++++++++------
net/sched/sch_api.c | 7 -----
net/sched/sch_atm.c | 24 ++--------------
net/sched/sch_cbq.c | 37 +------------------------
net/sched/sch_dsmark.c | 23 +---------------
net/sched/sch_fifo.c | 2 -
net/sched/sch_generic.c | 21 +-------------
net/sched/sch_gred.c | 21 --------------
net/sched/sch_hfsc.c | 30 ++------------------
net/sched/sch_htb.c | 46 +------------------------------
net/sched/sch_multiq.c | 38 +------------------------
net/sched/sch_netem.c | 29 +++-----------------
net/sched/sch_prio.c | 31 +--------------------
net/sched/sch_red.c | 20 +-------------
net/sched/sch_sfq.c | 66 ---------------------------------------------
net/sched/sch_tbf.c | 23 +--------------
net/sched/sch_teql.c | 11 -------
17 files changed, 36 insertions(+), 416 deletions(-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 9:52 [PATCH 00/14]: Killing qdisc->ops->requeue() Jarek Poplawski
@ 2008-10-14 11:39 ` Patrick McHardy
2008-10-14 12:26 ` Jarek Poplawski
2008-10-14 16:41 ` Alexander Duyck
1 sibling, 1 reply; 16+ messages in thread
From: Patrick McHardy @ 2008-10-14 11:39 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, netdev
Jarek Poplawski wrote:
> The aim of this patch-set is to finish changes proposed by David S.
> Miller in his patch-set with the same subject from Mon, 18 Aug 2008.
> The first two patches were applied with some modifications, so, to
> apply the rest, there were needed some changes.
>
> Original David's patches include additional info, but signed-off-by
> is removed because of changed context. I expect they will be merged
> and signed off by David as an author, anyway.
>
> The qdisc->requeue list idea is to limit requeuing to one level only,
> so a parent can requeue to its child only. This list is then tried
> first while dequeuing (qdisc_dequeue()), except at the top level,
> so packets could be requeued only by qdiscs, not by qdisc_restart()
> after xmit errors.
I didn't follow the original discussion, but I'm wondering what
the reasoning is why these patches won't have negative impact
on latency. Consider these two scenarios with HFSC or TBF:
current situation:
- packet is dequeued and sent
- next packet is peeked at for calculating the deadline
- watchdog is scheduled
- higher priority packet arrives and is queued to inner qdisc
- dequeue is called again, qdisc is overlimit, so peeks again
- watchdog is rescheduled based on higher priority packet
without ->requeue:
- packet is dequeued and sent
- next packet is peeked at for calculating the deadline and
put into private "requeue" queue
- watchdog is scheduled
- higher priority packet arrives and is queued to inner qdisc
- dequeue is called again, qdisc is overlimit, so peeks again
- higher priority packet doesn't affect watchdog rescheduling
since we still have one in the private queue
- lower priority packet is sent, assuming qdisc is overlimit
watchdog is then rescheduled based on higher priority packet
The end result is that the worst case latency for a packet increases
by a full packet transmission time. This may not matter much for high
bandwidth connections, but for f.i. with a 1mbit connection it adds a
full 12ms for a MTU of 1500, which is clearly in the noticable range.
I'm not opposed to killing top-level ->requeue since in that case
the qdisc has already decided to send the packet and if it affects
latency, the qdisc is misconfigured to use too much bandwidth.
Qdisc' use of ->requeue can only be removed without bad side effects
for the CBQ case of overlimit handling, it shouldn't matter much
since CBQ is not very accurate anyways. For the ->peek case (HFSC,
TBF, I think also netem) we really need the peek semantic to avoid
these side effects.
It should actually be pretty easy because for every ->enqueue call,
there is at least one immediately following ->dequeue call, which
gives an upper qdisc a chance to reschedule the watchdog when
conditions change. So what should work is having the requeue-queue
(actually, just an skb pointer) within the innermost qdisc instead
of one level higher, as in your patches.
On a ->peek operation, the qdisc would simply do what is currently
done in ->dequeue, but instead of removing the packet from its
private queues, it would set the pointer to point to the chosen
packet and return it to the upper qdisc. The upper qdisc can use
this for watchdog scheduling. If the next event is a dequeue event
(meaning the watchdog expired), it removes the peeked packet from
the private queues and returns it to the upper qdisc again. If the
next event is an enqueue event, it can replace the pointer
unconditionally since the upper qdisc will immediately call
->dequeue or ->peek again, giving it a chance to reschedule based
on the changed conditions.
So the implementation would probably roughly look like this:
- split ->dequeue into a queue and packet selection operation, setting
the above mentioned pointer, and an actual dequeue operation to
remove the selected packet from the queue.
- the queue and packet selection operation is at the same time the
->peek operation
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 11:39 ` Patrick McHardy
@ 2008-10-14 12:26 ` Jarek Poplawski
2008-10-14 12:32 ` Patrick McHardy
0 siblings, 1 reply; 16+ messages in thread
From: Jarek Poplawski @ 2008-10-14 12:26 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, netdev
On Tue, Oct 14, 2008 at 01:39:23PM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
>> The aim of this patch-set is to finish changes proposed by David S.
>> Miller in his patch-set with the same subject from Mon, 18 Aug 2008.
>> The first two patches were applied with some modifications, so, to
>> apply the rest, there were needed some changes.
>>
>> Original David's patches include additional info, but signed-off-by
>> is removed because of changed context. I expect they will be merged
>> and signed off by David as an author, anyway.
>>
>> The qdisc->requeue list idea is to limit requeuing to one level only,
>> so a parent can requeue to its child only. This list is then tried
>> first while dequeuing (qdisc_dequeue()), except at the top level,
>> so packets could be requeued only by qdiscs, not by qdisc_restart()
>> after xmit errors.
>
> I didn't follow the original discussion, but I'm wondering what
> the reasoning is why these patches won't have negative impact
> on latency. Consider these two scenarios with HFSC or TBF:
Hmm... What a pity you couldn't write this a bit earlier, because I've
just waited with this part for some distinct cons of this solution.
Alas, I can't analyze your concerns at the moment, and I'll try to
reply in the evening yet, but my idea was this all shouldn't make
(almost) any visible change just for HFSC, so if it's otherwise,
something went wrong. IMHO, with this solution hfsc_requeue() way is
simply realized as a standard now, but I can miss something.
Thanks for now,
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 12:26 ` Jarek Poplawski
@ 2008-10-14 12:32 ` Patrick McHardy
2008-10-14 17:56 ` Jarek Poplawski
0 siblings, 1 reply; 16+ messages in thread
From: Patrick McHardy @ 2008-10-14 12:32 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, netdev
Jarek Poplawski wrote:
> On Tue, Oct 14, 2008 at 01:39:23PM +0200, Patrick McHardy wrote:
>> Jarek Poplawski wrote:
>>> The aim of this patch-set is to finish changes proposed by David S.
>>> Miller in his patch-set with the same subject from Mon, 18 Aug 2008.
>>> The first two patches were applied with some modifications, so, to
>>> apply the rest, there were needed some changes.
>>>
>>> Original David's patches include additional info, but signed-off-by
>>> is removed because of changed context. I expect they will be merged
>>> and signed off by David as an author, anyway.
>>>
>>> The qdisc->requeue list idea is to limit requeuing to one level only,
>>> so a parent can requeue to its child only. This list is then tried
>>> first while dequeuing (qdisc_dequeue()), except at the top level,
>>> so packets could be requeued only by qdiscs, not by qdisc_restart()
>>> after xmit errors.
>> I didn't follow the original discussion, but I'm wondering what
>> the reasoning is why these patches won't have negative impact
>> on latency. Consider these two scenarios with HFSC or TBF:
>
> Hmm... What a pity you couldn't write this a bit earlier, because I've
> just waited with this part for some distinct cons of this solution.
I'm sorry for any wasted effort, I think I mentioned this one or
two month ago, but wasn't able to further participate in the
discussion because I was busy with other things.
> Alas, I can't analyze your concerns at the moment, and I'll try to
> reply in the evening yet, but my idea was this all shouldn't make
> (almost) any visible change just for HFSC, so if it's otherwise,
> something went wrong. IMHO, with this solution hfsc_requeue() way is
> simply realized as a standard now, but I can miss something.
The main difference results from the fact that higher priority
packets can't preempt "peeked" lower priority packets anymore.
The difference this causes obviously depends on the bandwidth,
but in my opinion the impact (as mentioned. 12ms delay for
1500b, 1mbit, rises linearly with bigger packets/less bandwidth)
is large enough to speak against these changes.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 9:52 [PATCH 00/14]: Killing qdisc->ops->requeue() Jarek Poplawski
2008-10-14 11:39 ` Patrick McHardy
@ 2008-10-14 16:41 ` Alexander Duyck
2008-10-14 18:37 ` Jarek Poplawski
2008-10-14 19:15 ` Jarek Poplawski
1 sibling, 2 replies; 16+ messages in thread
From: Alexander Duyck @ 2008-10-14 16:41 UTC (permalink / raw)
To: Jarek Poplawski, netdev@vger.kernel.org
Jarek Poplawski wrote:
> The aim of this patch-set is to finish changes proposed by David S.
> Miller in his patch-set with the same subject from Mon, 18 Aug 2008.
> The first two patches were applied with some modifications, so, to
> apply the rest, there were needed some changes.
>
> Original David's patches include additional info, but signed-off-by
> is removed because of changed context. I expect they will be merged
> and signed off by David as an author, anyway.
>
> The qdisc->requeue list idea is to limit requeuing to one level only,
> so a parent can requeue to its child only. This list is then tried
> first while dequeuing (qdisc_dequeue()), except at the top level,
> so packets could be requeued only by qdiscs, not by qdisc_restart()
> after xmit errors.
I still do not agree with this change. It essentially breaks multiq as
we are right back to all queues being stopped because of one packet in
the qdisc->requeue list.
I think if anything it seems like you guys actually found the cpu
performance issue a while back in the fact that the dev_requeue_skb was
calling __netif_schedule when requeuing on a stopped queue. That is the
one piece I would say needs to be changed so that you only call
__netif_schedule if the skb is not going to a stopped queue.
Thanks,
Alex
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 12:32 ` Patrick McHardy
@ 2008-10-14 17:56 ` Jarek Poplawski
2008-10-14 20:18 ` David Miller
2008-10-14 20:44 ` Patrick McHardy
0 siblings, 2 replies; 16+ messages in thread
From: Jarek Poplawski @ 2008-10-14 17:56 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, netdev
David,
I acknowledge Patrick's concerns, so please, consider this
patch-set as withdrawn. Thanks.
A little comment below:
Patrick McHardy wrote, On 10/14/2008 02:32 PM:
> Jarek Poplawski wrote:
...
> I'm sorry for any wasted effort, I think I mentioned this one or
> two month ago, but wasn't able to further participate in the
> discussion because I was busy with other things.
No problem, it seems I missed your point.
>> Alas, I can't analyze your concerns at the moment, and I'll try to
>> reply in the evening yet, but my idea was this all shouldn't make
>> (almost) any visible change just for HFSC, so if it's otherwise,
>> something went wrong. IMHO, with this solution hfsc_requeue() way is
>> simply realized as a standard now, but I can miss something.
>
> The main difference results from the fact that higher priority
> packets can't preempt "peeked" lower priority packets anymore.
> The difference this causes obviously depends on the bandwidth,
> but in my opinion the impact (as mentioned. 12ms delay for
> 1500b, 1mbit, rises linearly with bigger packets/less bandwidth)
> is large enough to speak against these changes.
Actually, I think I had similar doubts one or two months ago, but I
can remember mainly your discussion on peek with Herbert, and not
much about something like above, but probably I didn't pay enough
attention.
Anyway, the main source of my misleading seems to be HFSC... So do you
mean it's OK to use this kind of requeuing in hfsc_requeue(), but not
elsewhere? Maybe I miss something, but if some other qdisc is used as
a parent of HFSC, and does something like qdisc_peek_len(), it gets
just what you're against above?
Anyway, I'm glad you've found some time for these explanations,
especially since they're in accordance with my previous suspicions.
Thanks again,
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 16:41 ` Alexander Duyck
@ 2008-10-14 18:37 ` Jarek Poplawski
2008-10-14 18:41 ` Jarek Poplawski
2008-10-14 19:15 ` Jarek Poplawski
1 sibling, 1 reply; 16+ messages in thread
From: Jarek Poplawski @ 2008-10-14 18:37 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev@vger.kernel.org
On Tue, Oct 14, 2008 at 09:41:30AM -0700, Alexander Duyck wrote:
...
> I still do not agree with this change. It essentially breaks multiq as
> we are right back to all queues being stopped because of one packet in
> the qdisc->requeue list.
Good news! I've withdrawn this patch-set (for another reason yet).
> I think if anything it seems like you guys actually found the cpu
> performance issue a while back in the fact that the dev_requeue_skb was
> calling __netif_schedule when requeuing on a stopped queue. That is the
> one piece I would say needs to be changed so that you only call
> __netif_schedule if the skb is not going to a stopped queue.
Alexender, actually I've remembered your concerns, and I waited with
this mostly for you. Since you didn't write I thought you've changed
your mind in the meantime. Anyway, current changes are really minimal
and easy to revert, so I wonder why you don't try to convince us with
some patches and/or test results.
IMHO, even if you are right about some downsides of the current
"simplistic" solution, which I think are overestimated (since multiq
does it's own checks beforehand, especially when used as a root it
should almost never have it's packets requeued - otherwise, according
to David, it's a buggy driver), but it seems it wasn't only me thinking
your proposal adds too much code duplication.
And, after all, you seem to think everything should be adapted for the
sake of sch_multiq, which could be true in the future, but maybe we
should wait with this for users' requests?
Thanks,
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 18:37 ` Jarek Poplawski
@ 2008-10-14 18:41 ` Jarek Poplawski
0 siblings, 0 replies; 16+ messages in thread
From: Jarek Poplawski @ 2008-10-14 18:41 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev@vger.kernel.org
On Tue, Oct 14, 2008 at 08:37:30PM +0200, Jarek Poplawski wrote:
...
> Alexender, actually I've remembered your concerns, and I waited with
Sorry for this misspelling,
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 16:41 ` Alexander Duyck
2008-10-14 18:37 ` Jarek Poplawski
@ 2008-10-14 19:15 ` Jarek Poplawski
2008-10-14 20:37 ` Alexander Duyck
1 sibling, 1 reply; 16+ messages in thread
From: Jarek Poplawski @ 2008-10-14 19:15 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev@vger.kernel.org
On Tue, Oct 14, 2008 at 09:41:30AM -0700, Alexander Duyck wrote:
...
> I think if anything it seems like you guys actually found the cpu
> performance issue a while back in the fact that the dev_requeue_skb was
> calling __netif_schedule when requeuing on a stopped queue. That is the
> one piece I would say needs to be changed so that you only call
> __netif_schedule if the skb is not going to a stopped queue.
BTW, since one of the other "dreams" of killing requeuing failed, I
think this proposal is worth checking, but David was concerned about
some issues with buggy drivers after __netif_schedule() removing.
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 17:56 ` Jarek Poplawski
@ 2008-10-14 20:18 ` David Miller
2008-10-14 20:44 ` Patrick McHardy
1 sibling, 0 replies; 16+ messages in thread
From: David Miller @ 2008-10-14 20:18 UTC (permalink / raw)
To: jarkao2; +Cc: kaber, netdev
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Tue, 14 Oct 2008 19:56:49 +0200
> I acknowledge Patrick's concerns, so please, consider this
> patch-set as withdrawn. Thanks.
Ok, will scratch it from patchwork, thanks for the update.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 19:15 ` Jarek Poplawski
@ 2008-10-14 20:37 ` Alexander Duyck
2008-10-15 6:45 ` Jarek Poplawski
0 siblings, 1 reply; 16+ messages in thread
From: Alexander Duyck @ 2008-10-14 20:37 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: netdev@vger.kernel.org
Jarek Poplawski wrote:
> On Tue, Oct 14, 2008 at 09:41:30AM -0700, Alexander Duyck wrote:
> ...
>> I think if anything it seems like you guys actually found the cpu
>> performance issue a while back in the fact that the dev_requeue_skb was
>> calling __netif_schedule when requeuing on a stopped queue. That is the
>> one piece I would say needs to be changed so that you only call
>> __netif_schedule if the skb is not going to a stopped queue.
>
> BTW, since one of the other "dreams" of killing requeuing failed, I
> think this proposal is worth checking, but David was concerned about
> some issues with buggy drivers after __netif_schedule() removing.
>
> Jarek P.
I would say not to remove it. Just add a check to verify that the queue
the packet is bound for is not stopped prior to calling it. If the
queue is stopped it should be rescheduled by the wake_queue call when
the device has cleared the queue.
Unfortunately I am currently working on other projects so I don't really
have the time to create and test a patch for this. Hopefully my input
has proven useful.
Thanks,
Alex
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 17:56 ` Jarek Poplawski
2008-10-14 20:18 ` David Miller
@ 2008-10-14 20:44 ` Patrick McHardy
2008-10-15 8:27 ` Jarek Poplawski
1 sibling, 1 reply; 16+ messages in thread
From: Patrick McHardy @ 2008-10-14 20:44 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, netdev
[-- Attachment #1: Type: text/plain, Size: 1841 bytes --]
Jarek Poplawski wrote:
>> The main difference results from the fact that higher priority
>> packets can't preempt "peeked" lower priority packets anymore.
>> The difference this causes obviously depends on the bandwidth,
>> but in my opinion the impact (as mentioned. 12ms delay for
>> 1500b, 1mbit, rises linearly with bigger packets/less bandwidth)
>> is large enough to speak against these changes.
>
> Actually, I think I had similar doubts one or two months ago, but I
> can remember mainly your discussion on peek with Herbert, and not
> much about something like above, but probably I didn't pay enough
> attention.
>
> Anyway, the main source of my misleading seems to be HFSC... So do you
> mean it's OK to use this kind of requeuing in hfsc_requeue(), but not
> elsewhere? Maybe I miss something, but if some other qdisc is used as
> a parent of HFSC, and does something like qdisc_peek_len(), it gets
> just what you're against above?
I think we really need a peek operation since most qdiscs do have
some internal priorization. The question is whether all qdiscs need
it; I tend to think no. Plugging two non-work-conserving qdiscs
together doesn't make much sense, so we could just prevent this.
This means we would only need to add ->peek support to the
work-conserving qdiscs, which looks pretty easy in all cases.
We actually don't even have to prevent plugging two non-work-conserving
qdiscs, the ones that need the peek operation could just check whether
the inner qdisc supports it.
Just as a demonstration how easy adding a peek operation to the
work-conserving qdiscs actually is. It doesn't need to keep or change
any internal state in many cases thanks to the guarantee that the
packet will either be dequeued or, if another packet arrives, the
upper qdisc will immediately ->peek again to reevaluate the state.
[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2916 bytes --]
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index e556962..5462c50 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -431,6 +431,11 @@ static inline struct sk_buff *qdisc_dequeue_tail(struct Qdisc *sch)
return __qdisc_dequeue_tail(sch, &sch->q);
}
+static inline struct sk_buff *qdisc_peek_head(struct Qdisc *sch)
+{
+ return skb_peek(&sch->q);
+}
+
static inline int __qdisc_requeue(struct sk_buff *skb, struct Qdisc *sch,
struct sk_buff_head *list)
{
diff --git a/net/sched/sch_fifo.c b/net/sched/sch_fifo.c
index 23d258b..8825e88 100644
--- a/net/sched/sch_fifo.c
+++ b/net/sched/sch_fifo.c
@@ -83,6 +83,7 @@ struct Qdisc_ops pfifo_qdisc_ops __read_mostly = {
.priv_size = sizeof(struct fifo_sched_data),
.enqueue = pfifo_enqueue,
.dequeue = qdisc_dequeue_head,
+ .peek = qdisc_peek_head,
.requeue = qdisc_requeue,
.drop = qdisc_queue_drop,
.init = fifo_init,
@@ -98,6 +99,7 @@ struct Qdisc_ops bfifo_qdisc_ops __read_mostly = {
.priv_size = sizeof(struct fifo_sched_data),
.enqueue = bfifo_enqueue,
.dequeue = qdisc_dequeue_head,
+ .peek = qdisc_peek_head,
.requeue = qdisc_requeue,
.drop = qdisc_queue_drop,
.init = fifo_init,
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index a6697c6..8411472 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -120,6 +120,19 @@ prio_requeue(struct sk_buff *skb, struct Qdisc* sch)
return ret;
}
+static struct sk_buff *prio_peek(struct Qdisc *sch)
+{
+ struct prio_sched_data *q = qdisc_priv(sch);
+ int prio;
+
+ for (prio = 0; prio < q->bands; prio++) {
+ struct Qdisc *qdisc = q->queues[prio];
+ struct sk_buff *skb = qdisc->peek(qdisc);
+ if (skb)
+ return skb;
+ }
+ return NULL;
+}
static struct sk_buff *prio_dequeue(struct Qdisc* sch)
{
@@ -425,6 +438,7 @@ static struct Qdisc_ops prio_qdisc_ops __read_mostly = {
.priv_size = sizeof(struct prio_sched_data),
.enqueue = prio_enqueue,
.dequeue = prio_dequeue,
+ .peek = prio_peek,
.requeue = prio_requeue,
.drop = prio_drop,
.init = prio_init,
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 6e041d1..282a96c 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -391,8 +391,20 @@ sfq_requeue(struct sk_buff *skb, struct Qdisc *sch)
return NET_XMIT_CN;
}
+static struct sk_buff *
+sfq_peek(struct Qdisc *sch)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ struct sk_buff *skb;
+ sfq_index a;
+ /* No active slots */
+ if (q->tail == SFQ_DEPTH)
+ return NULL;
+ a = q->next[q->tail];
+ return skb_peek(&q->qs[a]);
+}
static struct sk_buff *
sfq_dequeue(struct Qdisc *sch)
@@ -624,6 +636,7 @@ static struct Qdisc_ops sfq_qdisc_ops __read_mostly = {
.priv_size = sizeof(struct sfq_sched_data),
.enqueue = sfq_enqueue,
.dequeue = sfq_dequeue,
+ .peek = sfq_peek,
.requeue = sfq_requeue,
.drop = sfq_drop,
.init = sfq_init,
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 20:37 ` Alexander Duyck
@ 2008-10-15 6:45 ` Jarek Poplawski
2008-10-15 7:19 ` Jarek Poplawski
0 siblings, 1 reply; 16+ messages in thread
From: Jarek Poplawski @ 2008-10-15 6:45 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev@vger.kernel.org
On Tue, Oct 14, 2008 at 01:37:14PM -0700, Alexander Duyck wrote:
> Jarek Poplawski wrote:
>> On Tue, Oct 14, 2008 at 09:41:30AM -0700, Alexander Duyck wrote:
>> ...
>>> I think if anything it seems like you guys actually found the cpu
>>> performance issue a while back in the fact that the dev_requeue_skb was
>>> calling __netif_schedule when requeuing on a stopped queue. That is the
>>> one piece I would say needs to be changed so that you only call
>>> __netif_schedule if the skb is not going to a stopped queue.
>>
>> BTW, since one of the other "dreams" of killing requeuing failed, I
>> think this proposal is worth checking, but David was concerned about
>> some issues with buggy drivers after __netif_schedule() removing.
>>
>> Jarek P.
>
> I would say not to remove it. Just add a check to verify that the queue
> the packet is bound for is not stopped prior to calling it. If the
> queue is stopped it should be rescheduled by the wake_queue call when
> the device has cleared the queue.
Sure, I meant "after __netif_schedule() call removing". Anyway, there
were a discussion on something like this:
http://marc.info/?l=linux-netdev&m=122197536025956&w=2
> Unfortunately I am currently working on other projects so I don't really
> have the time to create and test a patch for this. Hopefully my input
> has proven useful.
Without this __netif_schedule() call we should expect at least higher
cpu use here while packets are xmitted to a stopped queue compared to
older kernels or the current one (especially with multi qdisc/class/
filter configs), so first we should know this requeuing from the top is
really necessary, and I didn't see anything convincing about this yet.
Thanks,
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-15 6:45 ` Jarek Poplawski
@ 2008-10-15 7:19 ` Jarek Poplawski
0 siblings, 0 replies; 16+ messages in thread
From: Jarek Poplawski @ 2008-10-15 7:19 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev@vger.kernel.org
On Wed, Oct 15, 2008 at 06:45:22AM +0000, Jarek Poplawski wrote:
...
> Without this __netif_schedule() call we should expect at least higher
Should be:
"Even without this __netif_schedule() call..."
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-14 20:44 ` Patrick McHardy
@ 2008-10-15 8:27 ` Jarek Poplawski
2008-10-15 9:45 ` Patrick McHardy
0 siblings, 1 reply; 16+ messages in thread
From: Jarek Poplawski @ 2008-10-15 8:27 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, netdev
On Tue, Oct 14, 2008 at 10:44:13PM +0200, Patrick McHardy wrote:
...
> I think we really need a peek operation since most qdiscs do have
> some internal priorization. The question is whether all qdiscs need
> it; I tend to think no.
Looking at qdisc_peek_len() seems to confirm a peek would be useful.
But since this all started from the question if ->requeue() could be
killed or simplified, I wonder if adding this peek could really help
with this problem without too much reworking. It looks like at least
sch_netem would still need more.
But if it's rather about adding something new and useful I'm OK with
this. Anyway, I need some time to rethink this one and your previous
description.
Thanks,
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/14]: Killing qdisc->ops->requeue().
2008-10-15 8:27 ` Jarek Poplawski
@ 2008-10-15 9:45 ` Patrick McHardy
0 siblings, 0 replies; 16+ messages in thread
From: Patrick McHardy @ 2008-10-15 9:45 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, netdev
Jarek Poplawski wrote:
> On Tue, Oct 14, 2008 at 10:44:13PM +0200, Patrick McHardy wrote:
> ...
>> I think we really need a peek operation since most qdiscs do have
>> some internal priorization. The question is whether all qdiscs need
>> it; I tend to think no.
>
> Looking at qdisc_peek_len() seems to confirm a peek would be useful.
> But since this all started from the question if ->requeue() could be
> killed or simplified, I wonder if adding this peek could really help
> with this problem without too much reworking. It looks like at least
> sch_netem would still need more.
Indeed. netem is really a special case, it would be good if we could
treat it as such without requiring this kind of support from all qdiscs.
An idea would be to use a second queue for reordering that is always
tried to dequeue first. That should behave identical to what it does
now, except when the inner qdisc does reordering or rate limiting
itself.
> But if it's rather about adding something new and useful I'm OK with
> this. Anyway, I need some time to rethink this one and your previous
> description.
The main idea was to get rid of ->requeue, but it would also simplify
things slightly for HFSC, TBF and DRR (unsubmitted patch of mine).
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2008-10-15 9:45 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-10-14 9:52 [PATCH 00/14]: Killing qdisc->ops->requeue() Jarek Poplawski
2008-10-14 11:39 ` Patrick McHardy
2008-10-14 12:26 ` Jarek Poplawski
2008-10-14 12:32 ` Patrick McHardy
2008-10-14 17:56 ` Jarek Poplawski
2008-10-14 20:18 ` David Miller
2008-10-14 20:44 ` Patrick McHardy
2008-10-15 8:27 ` Jarek Poplawski
2008-10-15 9:45 ` Patrick McHardy
2008-10-14 16:41 ` Alexander Duyck
2008-10-14 18:37 ` Jarek Poplawski
2008-10-14 18:41 ` Jarek Poplawski
2008-10-14 19:15 ` Jarek Poplawski
2008-10-14 20:37 ` Alexander Duyck
2008-10-15 6:45 ` Jarek Poplawski
2008-10-15 7:19 ` Jarek Poplawski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).