Patchwork [7/6] Re: [PATCH 2/6] pkt_sched: sch_htb: Consider used jiffies in htb_dequeue()

login
register
mail settings
Submitter Jarek Poplawski
Date Dec. 10, 2008, 9:35 a.m.
Message ID <20081210093532.GA7926@ff.dom.local>
Download mbox | patch
Permalink /patch/13144/
State Deferred
Delegated to: David Miller
Headers show

Comments

Jarek Poplawski - Dec. 10, 2008, 9:35 a.m.
On Wed, Dec 10, 2008 at 01:14:31AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Wed, 10 Dec 2008 09:11:26 +0000
> 
> > On Tue, Dec 09, 2008 at 10:35:45PM -0800, David Miller wrote:
> > > So once you work this stuff out please resubmit the rest.
> > 
> > Of course, I can resubmit 1 and 4 if you think it's really needed.
> 
> Please do, there were dependencies.  In fact patch 2 changes
> the very change you made in patch 1.

But patch 2 is nonexistent. (I've withdrawn this...)

> 
> I understand it's logical steps, but you could just do 1 and
> 2 in the same patch and it'd be OK.

OK, so I withdraw 1 and 4 too, and let's say this is No 7 (2in1).

Sorry for this mess,
Jarek P.

------------->
pkt_sched: sch_htb: Consider used jiffies in htb_do_events()

Next event time should consider jiffies used for recounting. Otherwise
qdisc_watchdog_schedule() triggers hrtimer immediately with the event
in the past, and may cause very high ksoftirqd cpu usage (if highres
is on).

There is also removed checking "event" for zero in htb_dequeue(): it's
always true in this place.

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 net/sched/sch_htb.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

--
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 - Dec. 10, 2008, 2:38 p.m.
Jarek Poplawski wrote:
> On Wed, Dec 10, 2008 at 01:14:31AM -0800, David Miller wrote:
>   
>
>> I understand it's logical steps, but you could just do 1 and
>> 2 in the same patch and it'd be OK.
>>     
>
> OK, so I withdraw 1 and 4 too, and let's say this is No 7 (2in1).
>   


Just FYI, I'm travelling today, so I'll need until tommorrow to
review this.

--
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 - Dec. 16, 2008, 11:57 p.m.
Patrick, I know you said you were travelling and very busy,
but Jarek has been waiting patiently for you to review
these two newly respun patches.

These are just rotting in my patch queue and I'd like to
do something with them.

Thanks.
--
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 - Dec. 17, 2008, 7:03 a.m.
On Tue, Dec 16, 2008 at 03:57:22PM -0800, David Miller wrote:
> 
> Patrick, I know you said you were travelling and very busy,
> but Jarek has been waiting patiently for you to review
> these two newly respun patches.
> 
> These are just rotting in my patch queue and I'd like to
> do something with them.

I guess Patrick has some doubts, but since these patches are not
critical, let's say I'll resend them within a few weeks.

Thanks,
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
David Miller - Dec. 17, 2008, 7:38 a.m.
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 17 Dec 2008 07:03:03 +0000

> On Tue, Dec 16, 2008 at 03:57:22PM -0800, David Miller wrote:
> > 
> > Patrick, I know you said you were travelling and very busy,
> > but Jarek has been waiting patiently for you to review
> > these two newly respun patches.
> > 
> > These are just rotting in my patch queue and I'd like to
> > do something with them.
> 
> I guess Patrick has some doubts, but since these patches are not
> critical, let's say I'll resend them within a few weeks.

Fair enough, I'll mark them deferred on patchwork
--
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 - Jan. 12, 2009, 6:56 a.m.
David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Wed, 17 Dec 2008 07:03:03 +0000
> 
>> On Tue, Dec 16, 2008 at 03:57:22PM -0800, David Miller wrote:
>>> Patrick, I know you said you were travelling and very busy,
>>> but Jarek has been waiting patiently for you to review
>>> these two newly respun patches.
>>>
>>> These are just rotting in my patch queue and I'd like to
>>> do something with them.
>> I guess Patrick has some doubts, but since these patches are not
>> critical, let's say I'll resend them within a few weeks.
> 
> Fair enough, I'll mark them deferred on patchwork

Sorry, I dropped the ball on this one. I still think scheduling
a work-queue or something else running in process context to
kick the queue once the scheduler had a chance to run would
be a better solution. But Jarek's patches are an improvement
to the current situation, so no objections from me.


--
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 - Jan. 12, 2009, 10:10 a.m.
On Mon, Jan 12, 2009 at 07:56:37AM +0100, Patrick McHardy wrote:
> David Miller wrote:
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> Date: Wed, 17 Dec 2008 07:03:03 +0000
>>
>>> On Tue, Dec 16, 2008 at 03:57:22PM -0800, David Miller wrote:
>>>> Patrick, I know you said you were travelling and very busy,
>>>> but Jarek has been waiting patiently for you to review
>>>> these two newly respun patches.
>>>>
>>>> These are just rotting in my patch queue and I'd like to
>>>> do something with them.
>>> I guess Patrick has some doubts, but since these patches are not
>>> critical, let's say I'll resend them within a few weeks.
>>
>> Fair enough, I'll mark them deferred on patchwork
>
> Sorry, I dropped the ball on this one. I still think scheduling
> a work-queue or something else running in process context to
> kick the queue once the scheduler had a chance to run would
> be a better solution. But Jarek's patches are an improvement
> to the current situation, so no objections from me.
>

Thanks for the review Patrick. As I wrote before, I'm not against
using a workqueue here: it's logically better, but I still think
this place is rather exception, so I'm not convinced we should
care so much adding better solution, but also some overhead when
cancelling this workqueue. But if it really bothers you, please
confirm, and I'll do it. BTW, I wonder if adding the old "too many
events" warning back wouldn't be more useful here.

David, I'm not sure you can still track these patches, so I'll
soon resend them (2 patches: #7/6 and 8/6).

Thanks,
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
Patrick McHardy - Jan. 12, 2009, 10:22 a.m.
Jarek Poplawski wrote:
> On Mon, Jan 12, 2009 at 07:56:37AM +0100, Patrick McHardy wrote:
>> Sorry, I dropped the ball on this one. I still think scheduling
>> a work-queue or something else running in process context to
>> kick the queue once the scheduler had a chance to run would
>> be a better solution. But Jarek's patches are an improvement
>> to the current situation, so no objections from me.
>>
> 
> Thanks for the review Patrick. As I wrote before, I'm not against
> using a workqueue here: it's logically better, but I still think
> this place is rather exception, so I'm not convinced we should
> care so much adding better solution, but also some overhead when
> cancelling this workqueue. But if it really bothers you, please
> confirm, and I'll do it.

It doesn't bother me :) I just think its the technical better
and also most likely code-wise cleaner solution to this problem.
Cancellation wouldn't be necessary since an unnecessary
netif_schedule() doesn't really matter.

It you don't mind adding the workqueue, I certainly would prefer
it, but I'm also fine with this patch. I don't have a HTB setup
or a testcase for this specific case, otherwise I'd simply do it
myself.

> BTW, I wonder if adding the old "too many
> events" warning back wouldn't be more useful here.

It would be good to notify the user and also have some indication
for this case when looking into bug reports. A counter or a (single)
printk would both be 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
Jarek Poplawski - Jan. 12, 2009, 10:29 a.m.
On Mon, Jan 12, 2009 at 10:10:47AM +0000, Jarek Poplawski wrote:
...
> David, I'm not sure you can still track these patches, so I'll
> soon resend them (2 patches: #7/6 and 8/6).

Maybe I should have mention this: there is nothing more except these 2
patches to merge from this thread.

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
David Miller - Jan. 12, 2009, 10:32 a.m.
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 12 Jan 2009 10:10:47 +0000

> David, I'm not sure you can still track these patches, so I'll
> soon resend them (2 patches: #7/6 and 8/6).

Thanks, please do that.
--
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 - Jan. 12, 2009, 10:59 a.m.
On Mon, Jan 12, 2009 at 02:32:50AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 12 Jan 2009 10:10:47 +0000
> 
> > David, I'm not sure you can still track these patches, so I'll
> > soon resend them (2 patches: #7/6 and 8/6).
> 
> Thanks, please do that.

I hope you've received them.

Thanks,
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
David Miller - Jan. 12, 2009, 11:04 a.m.
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 12 Jan 2009 10:59:03 +0000

> On Mon, Jan 12, 2009 at 02:32:50AM -0800, David Miller wrote:
> > From: Jarek Poplawski <jarkao2@gmail.com>
> > Date: Mon, 12 Jan 2009 10:10:47 +0000
> > 
> > > David, I'm not sure you can still track these patches, so I'll
> > > soon resend them (2 patches: #7/6 and 8/6).
> > 
> > Thanks, please do that.
> 
> I hope you've received them.

I did, thanks.
--
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 - Jan. 12, 2009, 11:08 a.m.
On Mon, Jan 12, 2009 at 11:22:47AM +0100, Patrick McHardy wrote:
...
> It doesn't bother me :) I just think its the technical better
> and also most likely code-wise cleaner solution to this problem.
> Cancellation wouldn't be necessary since an unnecessary
> netif_schedule() doesn't really matter.

Hmm... Do you mean during destroying... It's probably not very long,
but deleting and creating qdiscs especially for some virtual devices
can take longer I guess.

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
Patrick McHardy - Jan. 12, 2009, 1:10 p.m.
Jarek Poplawski wrote:
> On Mon, Jan 12, 2009 at 11:22:47AM +0100, Patrick McHardy wrote:
> ...
>> It doesn't bother me :) I just think its the technical better
>> and also most likely code-wise cleaner solution to this problem.
>> Cancellation wouldn't be necessary since an unnecessary
>> netif_schedule() doesn't really matter.
> 
> Hmm... Do you mean during destroying... It's probably not very long,
> but deleting and creating qdiscs especially for some virtual devices
> can take longer I guess.

I was referring to your statement "but also some overhead when
cancelling this workqueue" and assumed you meant during packet
processing. In the destruction path it really doesn't matter at
all.
--
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

Patch

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 5070643..9ca8a26 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -685,8 +685,8 @@  static psched_time_t htb_do_events(struct htb_sched *q, int level)
 		if (cl->cmode != HTB_CAN_SEND)
 			htb_add_to_wait_tree(q, cl, diff);
 	}
-	/* too much load - let's continue on next jiffie */
-	return q->now + PSCHED_TICKS_PER_SEC / HZ;
+	/* too much load - let's continue on next jiffie (including above) */
+	return q->now + 2 * PSCHED_TICKS_PER_SEC / HZ;
 }
 
 /* Returns class->node+prio from id-tree where classe's id is >= id. NULL
@@ -873,7 +873,7 @@  static struct sk_buff *htb_dequeue(struct Qdisc *sch)
 		} else
 			event = q->near_ev_cache[level];
 
-		if (event && next_event > event)
+		if (next_event > event)
 			next_event = event;
 
 		m = ~q->row_mask[level];