Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Andrew Morton]

On Sun, 24 Jun 2007 21:57:19 -0700 (PDT) bugme-daemon@bugzilla.kernel.org wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=8668
> 
>            Summary: HTB Deadlock
>            Product: Networking
>            Version: 2.5
>      KernelVersion: 2.6.19.7
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: Other
>         AssignedTo: acme@ghostprotocols.net
>         ReportedBy: ranko@spidernet.net
> 
> 
> Most recent kernel where this bug did not occur:
> Distribution:
> Hardware Environment:
> Software Environment:
> Problem Description:
> Greetings,
> 
> I've been experiencing problems with HTB where the whole machine locks
> up. This usually happens when the whole qdisc is being removed and
> occasionally when a leaf is being removed.
> 
> Common is that it always happens when some sort of removal is in
> progress.
> 
> Console output I have captured is at the end of this message. The same
> behavior exists from vanilla 2.6.19.7 and above. It is possible that the
> problem also exist in the earlier versions however I did not go further
> back.
> 
> I also believe I have found where the actual problem is:
> 
> qdisc_destroy() function is always called with dev->queue_lock locked.
> htb_destroy() function up the stack is using del_timer_sync() call to
> deactivate HTB qdisc timers. 

yep, I would agree with that analysis.  del_timer_sync() under a lock is
quite dangerous in this regard.

If the (misspelled) comment over htb_destroy() is true, current mainline
appears still to have this bug.


> >From the comments in the source where del_timer_sync() is defined:
> 
> ---copy/paste---
> /**
>  * del_timer_sync - deactivate a timer and wait for the handler to finish.
>  * @timer: the timer to be deactivated
>  *
>  * This function only differs from del_timer() on SMP: besides deactivating
>  * the timer it also makes sure the handler has finished executing on other
>  * CPUs.
>  *
>  * Synchronization rules: Callers must prevent restarting of the timer,
>  * otherwise this function is meaningless. It must not be called from
>  * interrupt contexts. The caller must not hold locks which would prevent
>  * completion of the timer's handler. The timer's handler must not call
>  * add_timer_on(). Upon exit the timer is not queued and the handler is
>  * not running on any CPU.
>  *
>  * The function returns whether it has deactivated a pending timer or not.
>  */
> ---copy/paste---
> 
> Now, htb_rate_timer() does exactly what appears to be the source of the
> problem - it tries obtain dev->queue_lock - and given the right moment
> (timer fired handler while qdisc_destroy was holding the lock) - system
> locks up - del_timer_sync is waiting for handler to finish while the
> handler is waiting for the dev->queue_lock.
> 
> Of course I could also be completely wrong here and missing something
> not so obvious.
> 
> I could also attempt to fix this but I haven't dealt with this code in
> the past so I was hoping someone with better insight might just have an
> elegant solution up his sleeve.
> 
> Best regards,
> 
> Ranko
> 
> PS: If this is not the right place for this report - please let me
> know.
> 
> -----------CONSOLE (2.6.19.7)-----------
> BUG: soft lockup detected on CPU#3!
>  [<c013c890>] softlockup_tick+0x93/0xc2
>  [<c0127585>] update_process_times+0x26/0x5c
>  [<c0111cd5>] smp_apic_timer_interrupt+0x97/0xb2
>  [<c0104373>] apic_timer_interrupt+0x1f/0x24
>  [<c02e007b>] klist_next+0x4/0x8a
>  [<c02e2570>] _spin_unlock_irqrestore+0xa/0xc
>  [<c012729b>] try_to_del_timer_sync+0x47/0x4f
>  [<c01272b1>] del_timer_sync+0xe/0x14
>  [<f8b8a85b>] htb_destroy+0x20/0x7b [sch_htb]
>  [<c028f196>] qdisc_destroy+0x44/0x8d
>  [<f8b89645>] htb_destroy_class+0xd0/0x12d [sch_htb]
>  [<f8b895c7>] htb_destroy_class+0x52/0x12d [sch_htb]
>  [<f8b8a87a>] htb_destroy+0x3f/0x7b [sch_htb]
>  [<c028f196>] qdisc_destroy+0x44/0x8d
>  [<f8b89645>] htb_destroy_class+0xd0/0x12d [sch_htb]
>  [<f8b895c7>] htb_destroy_class+0x52/0x12d [sch_htb]
>  [<f8b8a87a>] htb_destroy+0x3f/0x7b [sch_htb]
>  [<c028f196>] qdisc_destroy+0x44/0x8d
>  [<c0290ba9>] tc_get_qdisc+0x1a3/0x1ef
>  [<c0290a06>] tc_get_qdisc+0x0/0x1ef
>  [<c028a366>] rtnetlink_rcv_msg+0x158/0x215
>  [<c028a20e>] rtnetlink_rcv_msg+0x0/0x215
>  [<c0294598>] netlink_run_queue+0x88/0x11d
>  [<c028a1c0>] rtnetlink_rcv+0x26/0x42
>  [<c0294b0c>] netlink_data_ready+0x12/0x54
>  [<c0293843>] netlink_sendskb+0x1c/0x33
>  [<c0294a11>] netlink_sendmsg+0x1ee/0x2d7
>  [<c0278ff7>] sock_sendmsg+0xe5/0x100
>  [<c01306b9>] autoremove_wake_function+0x0/0x37
>  [<c01306b9>] autoremove_wake_function+0x0/0x37
>  [<c0278ff7>] sock_sendmsg+0xe5/0x100
>  [<c01cd8be>] copy_from_user+0x33/0x69
>  [<c027913f>] sys_sendmsg+0x12d/0x243
>  [<c02e2564>] _read_unlock_irq+0x5/0x7
>  [<c013fb2b>] find_get_page+0x37/0x42
>  [<c01423dd>] filemap_nopage+0x30c/0x3a3
>  [<c014bb99>] __handle_mm_fault+0x21c/0x943
>  [<c02e24c5>] _spin_unlock_bh+0x5/0xd
>  [<c027b475>] sock_setsockopt+0x63/0x59d
>  [<c0151801>] anon_vma_prepare+0x1b/0xcb
>  [<c027a2ea>] sys_socketcall+0x24f/0x271
>  [<c02e3ad0>] do_page_fault+0x0/0x600
>  [<c01038f1>] sysenter_past_esp+0x56/0x79
>  =======================
> BUG: soft lockup detected on CPU#1!
>  [<c013c890>] softlockup_tick+0x93/0xc2
>  [<c0127585>] update_process_times+0x26/0x5c
>  [<c0111cd5>] smp_apic_timer_interrupt+0x97/0xb2
>  [<c0104373>] apic_timer_interrupt+0x1f/0x24
>  [<c01c007b>] blk_do_ordered+0x70/0x27e
>  [<c01ce788>] _raw_spin_lock+0xaa/0x13e
>  [<f8b8b422>] htb_rate_timer+0x18/0xc4 [sch_htb]
>  [<c0127539>] run_timer_softirq+0x163/0x189
>  [<f8b8b40a>] htb_rate_timer+0x0/0xc4 [sch_htb]
>  [<c0123315>] __do_softirq+0x70/0xdb
>  [<c01233bb>] do_softirq+0x3b/0x42
>  [<c0111cda>] smp_apic_timer_interrupt+0x9c/0xb2
>  [<c0104373>] apic_timer_interrupt+0x1f/0x24
>  [<c0101cc3>] mwait_idle_with_hints+0x3b/0x3f
>  [<c0101cd3>] mwait_idle+0xc/0x1b
>  [<c010271c>] cpu_idle+0x63/0x79
>  =======================
> BUG: soft lockup detected on CPU#2!
>  [<c013c890>] softlockup_tick+0x93/0xc2
>  [<c0127585>] update_process_times+0x26/0x5c
>  [<c0111cd5>] smp_apic_timer_interrupt+0x97/0xb2
>  [<c0104373>] apic_timer_interrupt+0x1f/0x24
>  [<c01c007b>] blk_do_ordered+0x70/0x27e
>  [<c01ce788>] _raw_spin_lock+0xaa/0x13e
>  [<c02846df>] dev_queue_xmit+0x53/0x2e4
>  [<c0286e20>] neigh_connected_output+0x80/0xa0
>  [<c02a213a>] ip_output+0x1b5/0x24b
>  [<c02a0b56>] ip_finish_output+0x0/0x192
>  [<c029dfef>] ip_forward+0x1c8/0x2b9
>  [<c029ddf0>] ip_forward_finish+0x0/0x37
>  [<c029c962>] ip_rcv+0x2a5/0x538
>  [<c029c100>] ip_rcv_finish+0x0/0x2aa
>  [<c027f3bc>] __netdev_alloc_skb+0x12/0x2a
>  [<c029c6bd>] ip_rcv+0x0/0x538
>  [<c0282a1e>] netif_receive_skb+0x218/0x318
>  [<c0270008>] bitmap_get_counter+0x41/0x1e6
>  [<f8a6146d>] e1000_clean_rx_irq+0x12c/0x4ef [e1000]
>  [<f8a61341>] e1000_clean_rx_irq+0x0/0x4ef [e1000]
>  [<f8a60612>] e1000_clean+0xe5/0x130 [e1000]
>  [<c0284573>] net_rx_action+0xbc/0x1d5
>  [<c0123315>] __do_softirq+0x70/0xdb
>  [<c01233bb>] do_softirq+0x3b/0x42
>  [<c01058c2>] do_IRQ+0x6c/0xda
>  [<c01042e2>] common_interrupt+0x1a/0x20
>  [<c0101cc3>] mwait_idle_with_hints+0x3b/0x3f
>  [<c0101cd3>] mwait_idle+0xc/0x1b
>  [<c010271c>] cpu_idle+0x63/0x79
>  =======================
> BUG: soft lockup detected on CPU#0!
>  [<c013c890>] softlockup_tick+0x93/0xc2
>  [<c0127585>] update_process_times+0x26/0x5c
>  [<c0111cd5>] smp_apic_timer_interrupt+0x97/0xb2
>  [<c0104373>] apic_timer_interrupt+0x1f/0x24
>  [<c01cd2eb>] delay_tsc+0x7/0x13
>  [<c01cd323>] __delay+0x6/0x7
>  [<c01ce796>] _raw_spin_lock+0xb8/0x13e
>  [<c02846df>] dev_queue_xmit+0x53/0x2e4
>  [<c0286e20>] neigh_connected_output+0x80/0xa0
>  [<c02a213a>] ip_output+0x1b5/0x24b
>  [<c02a0b56>] ip_finish_output+0x0/0x192
>  [<c029dfef>] ip_forward+0x1c8/0x2b9
>  [<c029ddf0>] ip_forward_finish+0x0/0x37
>  [<c029c962>] ip_rcv+0x2a5/0x538
>  [<c029c100>] ip_rcv_finish+0x0/0x2aa
>  [<c027e774>] __alloc_skb+0x47/0xf3
>  [<c029c6bd>] ip_rcv+0x0/0x538
>  [<c0282a1e>] netif_receive_skb+0x218/0x318
>  [<c0270008>] bitmap_get_counter+0x41/0x1e6
>  [<f88fac1d>] tg3_poll+0x6d3/0x906 [tg3]
>  [<c0284573>] net_rx_action+0xbc/0x1d5
>  [<c0123315>] __do_softirq+0x70/0xdb
>  [<c01233bb>] do_softirq+0x3b/0x42
>  [<c01058c2>] do_IRQ+0x6c/0xda
>  [<c01042e2>] common_interrupt+0x1a/0x20
>  [<c0101cc3>] mwait_idle_with_hints+0x3b/0x3f
>  [<c0101cd3>] mwait_idle+0xc/0x1b
>  [<c010271c>] cpu_idle+0x63/0x79
>  [<c03a9780>] start_kernel+0x353/0x423
>  [<c03a91cd>] unknown_bootoption+0x0/0x260
>  =======================
> -----------CONSOLE-----------
> 
> Steps to reproduce:
> 
> 
> -- 
> Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You are on the CC list for the bug, or are watching someone who is.

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Patrick McHardy]

Andrew Morton wrote:
> On Sun, 24 Jun 2007 21:57:19 -0700 (PDT) bugme-daemon@bugzilla.kernel.org wrote:
> 
>>I've been experiencing problems with HTB where the whole machine locks
>>up. This usually happens when the whole qdisc is being removed and
>>occasionally when a leaf is being removed.


It shouldn't happen when leaves are removed, you might be running
into some endless dequeue loops however that got fixed in 2.6.20.

>>Common is that it always happens when some sort of removal is in
>>progress.
>>
>>Console output I have captured is at the end of this message. The same
>>behavior exists from vanilla 2.6.19.7 and above. It is possible that the
>>problem also exist in the earlier versions however I did not go further
>>back.
>>
>>I also believe I have found where the actual problem is:
>>
>>qdisc_destroy() function is always called with dev->queue_lock locked.
>>htb_destroy() function up the stack is using del_timer_sync() call to
>>deactivate HTB qdisc timers. 
> 
> 
> yep, I would agree with that analysis.  del_timer_sync() under a lock is
> quite dangerous in this regard.
> 
> If the (misspelled) comment over htb_destroy() is true, current mainline
> appears still to have this bug.


It is. This patch I had originally planned for 2.6.23 switches HTB
to the generic estimator, which shouldn't suffer from this.

Ranko, can you try if it fixes your timer problem?

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

Patrick McHardy wrote:
> It is. This patch I had originally planned for 2.6.23 switches HTB
> to the generic estimator, which shouldn't suffer from this.
> 
> Ranko, can you try if it fixes your timer problem?


Forgot the patch ..

[-- Attachment #2: 03.diff --]
[-- Type: text/x-diff, Size: 6213 bytes --]

[NET_SCHED]: sch_htb: use generic estimator

Use the generic estimator instead of reimplementing (parts of) it.
For compatibility always create a default estimator for new classes.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 156d3d463446258c3e72b9dc10238e807bfeec71
tree a136f5576a0ca3066c200c577e559097e6a5d4b8
parent 831e7e7451b662679e0e3cf56e7c070a027de306
author Patrick McHardy <kaber@trash.net> Tue, 19 Jun 2007 23:21:00 +0200
committer Patrick McHardy <kaber@trash.net> Tue, 19 Jun 2007 23:21:00 +0200

 net/sched/sch_htb.c |   85 ++++++++++++++-------------------------------------
 1 files changed, 24 insertions(+), 61 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 035788c..26f81b8 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -69,8 +69,6 @@
 */
 
 #define HTB_HSIZE 16		/* classid hash size */
-#define HTB_EWMAC 2		/* rate average over HTB_EWMAC*HTB_HSIZE sec */
-#define HTB_RATECM 1		/* whether to use rate computer */
 #define HTB_HYSTERESIS 1	/* whether to use mode hysteresis for speedup */
 #define HTB_VER 0x30011		/* major must be matched with number suplied by TC as version */
 
@@ -95,12 +93,6 @@ struct htb_class {
 	struct tc_htb_xstats xstats;	/* our special stats */
 	int refcnt;		/* usage count of this class */
 
-#ifdef HTB_RATECM
-	/* rate measurement counters */
-	unsigned long rate_bytes, sum_bytes;
-	unsigned long rate_packets, sum_packets;
-#endif
-
 	/* topology */
 	int level;		/* our level (see above) */
 	struct htb_class *parent;	/* parent class */
@@ -194,10 +186,6 @@ struct htb_sched {
 	int rate2quantum;	/* quant = rate / rate2quantum */
 	psched_time_t now;	/* cached dequeue time */
 	struct qdisc_watchdog watchdog;
-#ifdef HTB_RATECM
-	struct timer_list rttim;	/* rate computer timer */
-	int recmp_bucket;	/* which hash bucket to recompute next */
-#endif
 
 	/* non shaped skbs; let them go directly thru */
 	struct sk_buff_head direct_queue;
@@ -677,34 +665,6 @@ static int htb_requeue(struct sk_buff *skb, struct Qdisc *sch)
 	return NET_XMIT_SUCCESS;
 }
 
-#ifdef HTB_RATECM
-#define RT_GEN(D,R) R+=D-(R/HTB_EWMAC);D=0
-static void htb_rate_timer(unsigned long arg)
-{
-	struct Qdisc *sch = (struct Qdisc *)arg;
-	struct htb_sched *q = qdisc_priv(sch);
-	struct hlist_node *p;
-	struct htb_class *cl;
-
-
-	/* lock queue so that we can muck with it */
-	spin_lock_bh(&sch->dev->queue_lock);
-
-	q->rttim.expires = jiffies + HZ;
-	add_timer(&q->rttim);
-
-	/* scan and recompute one bucket at time */
-	if (++q->recmp_bucket >= HTB_HSIZE)
-		q->recmp_bucket = 0;
-
-	hlist_for_each_entry(cl,p, q->hash + q->recmp_bucket, hlist) {
-		RT_GEN(cl->sum_bytes, cl->rate_bytes);
-		RT_GEN(cl->sum_packets, cl->rate_packets);
-	}
-	spin_unlock_bh(&sch->dev->queue_lock);
-}
-#endif
-
 /**
  * htb_charge_class - charges amount "bytes" to leaf and ancestors
  *
@@ -750,11 +710,6 @@ static void htb_charge_class(struct htb_sched *q, struct htb_class *cl,
 			if (cl->cmode != HTB_CAN_SEND)
 				htb_add_to_wait_tree(q, cl, diff);
 		}
-#ifdef HTB_RATECM
-		/* update rate counters */
-		cl->sum_bytes += bytes;
-		cl->sum_packets++;
-#endif
 
 		/* update byte stats except for leaves which are already updated */
 		if (cl->level) {
@@ -1095,13 +1050,6 @@ static int htb_init(struct Qdisc *sch, struct rtattr *opt)
 	if (q->direct_qlen < 2)	/* some devices have zero tx_queue_len */
 		q->direct_qlen = 2;
 
-#ifdef HTB_RATECM
-	init_timer(&q->rttim);
-	q->rttim.function = htb_rate_timer;
-	q->rttim.data = (unsigned long)sch;
-	q->rttim.expires = jiffies + HZ;
-	add_timer(&q->rttim);
-#endif
 	if ((q->rate2quantum = gopt->rate2quantum) < 1)
 		q->rate2quantum = 1;
 	q->defcls = gopt->defcls;
@@ -1175,11 +1123,6 @@ htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 {
 	struct htb_class *cl = (struct htb_class *)arg;
 
-#ifdef HTB_RATECM
-	cl->rate_est.bps = cl->rate_bytes / (HTB_EWMAC * HTB_HSIZE);
-	cl->rate_est.pps = cl->rate_packets / (HTB_EWMAC * HTB_HSIZE);
-#endif
-
 	if (!cl->level && cl->un.leaf.q)
 		cl->qstats.qlen = cl->un.leaf.q->q.qlen;
 	cl->xstats.tokens = cl->tokens;
@@ -1277,6 +1220,7 @@ static void htb_destroy_class(struct Qdisc *sch, struct htb_class *cl)
 		BUG_TRAP(cl->un.leaf.q);
 		qdisc_destroy(cl->un.leaf.q);
 	}
+	gen_kill_estimator(&cl->bstats, &cl->rate_est);
 	qdisc_put_rtab(cl->rate);
 	qdisc_put_rtab(cl->ceil);
 
@@ -1305,9 +1249,6 @@ static void htb_destroy(struct Qdisc *sch)
 	struct htb_sched *q = qdisc_priv(sch);
 
 	qdisc_watchdog_cancel(&q->watchdog);
-#ifdef HTB_RATECM
-	del_timer_sync(&q->rttim);
-#endif
 	/* This line used to be after htb_destroy_class call below
 	   and surprisingly it worked in 2.4. But it must precede it
 	   because filter need its target class alive to be able to call
@@ -1403,6 +1344,20 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 	if (!cl) {		/* new class */
 		struct Qdisc *new_q;
 		int prio;
+		struct {
+			struct rtattr		rta;
+			struct gnet_estimator	opt;
+		} est = {
+			.rta = {
+				.rta_len	= RTA_LENGTH(sizeof(est.opt)),
+				.rta_type	= TCA_RATE,
+			},
+			.opt = {
+				/* 4s interval, 16s averaging constant */
+				.interval	= 2,
+				.ewma_log	= 2,
+			},
+		};
 
 		/* check for valid classid */
 		if (!classid || TC_H_MAJ(classid ^ sch->handle)
@@ -1418,6 +1373,9 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 		if ((cl = kzalloc(sizeof(*cl), GFP_KERNEL)) == NULL)
 			goto failure;
 
+		gen_new_estimator(&cl->bstats, &cl->rate_est,
+				  &sch->dev->queue_lock,
+				  tca[TCA_RATE-1] ? : &est.rta);
 		cl->refcnt = 1;
 		INIT_LIST_HEAD(&cl->sibling);
 		INIT_HLIST_NODE(&cl->hlist);
@@ -1469,8 +1427,13 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 		hlist_add_head(&cl->hlist, q->hash + htb_hash(classid));
 		list_add_tail(&cl->sibling,
 			      parent ? &parent->children : &q->root);
-	} else
+	} else {
+		if (tca[TCA_RATE-1])
+			gen_replace_estimator(&cl->bstats, &cl->rate_est,
+					      &sch->dev->queue_lock,
+					      tca[TCA_RATE-1]);
 		sch_tree_lock(sch);
+	}
 
 	/* it used to be a nasty bug here, we have to check that node
 	   is really leaf before changing cl->un.leaf ! */

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Ranko Zivojnovic]

On Mon, 2007-06-25 at 11:30 +0200, Patrick McHardy wrote:
> Patrick McHardy wrote:
> > It is. This patch I had originally planned for 2.6.23 switches HTB
> > to the generic estimator, which shouldn't suffer from this.
> > 
> > Ranko, can you try if it fixes your timer problem?
> 
> 
> Forgot the patch ..

Will try it - yes.

R.

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Patrick McHardy]

Jarek Poplawski wrote:
> On 25-06-2007 11:28, Patrick McHardy wrote:
> ...
>   
>> It is. This patch I had originally planned for 2.6.23 switches HTB
>> to the generic estimator, which shouldn't suffer from this.
>>     
>
> BTW, maybe I look at this too short, but is this del_timer()
> in gen_kill_estimator() enough? I cannot see nothing against
> a timer just running and doing mod_timer() again...

Yes, but nothing bad can happen, the timer will find an empty
list and do nothing. It would make more sense to check for
an empty list before restarting the timer though.


Could you send a patch for that?

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Jarek Poplawski]

On 25-06-2007 11:28, Patrick McHardy wrote:
...
> It is. This patch I had originally planned for 2.6.23 switches HTB
> to the generic estimator, which shouldn't suffer from this.

BTW, maybe I look at this too short, but is this del_timer()
in gen_kill_estimator() enough? I cannot see nothing against
a timer just running and doing mod_timer() again...

Regards,
Jarek P.

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Jarek Poplawski]

On Wed, Jun 27, 2007 at 01:44:08PM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> >On 25-06-2007 11:28, Patrick McHardy wrote:
> >...
> >  
> >>It is. This patch I had originally planned for 2.6.23 switches HTB
> >>to the generic estimator, which shouldn't suffer from this.
> >>    
> >
> >BTW, maybe I look at this too short, but is this del_timer()
> >in gen_kill_estimator() enough? I cannot see nothing against
> >a timer just running and doing mod_timer() again...
> 
> Yes, but nothing bad can happen, the timer will find an empty
> list and do nothing. It would make more sense to check for
> an empty list before restarting the timer though.
> 
> 
> Could you send a patch for that?
> 

Probably I could, but it's your idea!

I look at this just now, and maybe it's enough for asking,
but definitely not enough for patch. I'll try to check this
more in the evening, so I could send something tomorrow.

So if it's not only about kindness, feel free to do it
sooner and I've no doubts  - better.

Thanks for so instant reply!
Jarek P.

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Jarek Poplawski]

On Wed, Jun 27, 2007 at 02:10:13PM +0200, Jarek Poplawski wrote:
...
- > So if it's not only about kindness, feel free to do it
+ > So if it's only about kindness, feel free to do it

Sorry!
Jarek P.

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Patrick McHardy]

Jarek Poplawski wrote:
> On Wed, Jun 27, 2007 at 01:44:08PM +0200, Patrick McHardy wrote:
>   
>>> BTW, maybe I look at this too short, but is this del_timer()
>>> in gen_kill_estimator() enough? I cannot see nothing against
>>> a timer just running and doing mod_timer() again...
>>>       
>> Yes, but nothing bad can happen, the timer will find an empty
>> list and do nothing. It would make more sense to check for
>> an empty list before restarting the timer though.
>>
>>
>> Could you send a patch for that?
>>
>>     
>
> Probably I could, but it's your idea!
>
> I look at this just now, and maybe it's enough for asking,
> but definitely not enough for patch. I'll try to check this
> more in the evening, so I could send something tomorrow.
>
> So if it's not only about kindness, feel free to do it
> sooner and I've no doubts  - better.

I can take care of it, no problem.

[NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

Patrick McHardy wrote:
> Jarek Poplawski wrote:
>> I look at this just now, and maybe it's enough for asking,
>> but definitely not enough for patch. I'll try to check this
>> more in the evening, so I could send something tomorrow.
>>
>> So if it's not only about kindness, feel free to do it
>> sooner and I've no doubts  - better.
>
> I can take care of it, no problem. 


OK, this patch should fix the Jarek noticed (and a few more).
It does not fix the original HTB problem though.




[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3180 bytes --]

[NET]: gen_estimator: fix locking and timer related bugs

As noticed by Jarek Poplawski <jarkao2@o2.pl>, the timer removal in
gen_kill_estimator races with the timer function rearming the timer.

Additionally there are a few more related problems that seem to be
relicts from the time when the estimator was qdisc specific and
could rely on the rtnl or dev->qdisc_lock:

- the check whether the list is empty and a timer needs to be started
  when adding a new estimator doesn't take the lock, so it races
  against concurrent additions, which can result in the timer getting
  added twice or getting reinitialized after getting added.

- the new estimator's next pointer is also set without holding the
  lock, again racing against concurrent additions with possible
  list corruption as a result.

- the timer deletion when killing an estimator is also not under
  the lock and races against timer arming when adding a new estimator.

Fix by holding the lock around the entire list addition and initial
timer arming. Timer removal is not done explicitly anymore, instead
the timer function only rearms the timer when there are still
estimators present.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 59b5997f78c3cf3366886969ac8e6b38100b30e9
tree 9b43ce1af21ad1c8817f1f2bc8291c0f60457a4e
parent 48d8d7ee5dd17c64833e0343ab4ae8ef01cc2648
author Patrick McHardy <kaber@trash.net> Wed, 27 Jun 2007 17:06:02 +0200
committer Patrick McHardy <kaber@trash.net> Wed, 27 Jun 2007 17:06:02 +0200

 net/core/gen_estimator.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
index 17daf4c..49a0bd3 100644
--- a/net/core/gen_estimator.c
+++ b/net/core/gen_estimator.c
@@ -127,8 +127,8 @@ static void est_timer(unsigned long arg)
 		e->rate_est->pps = (e->avpps+0x1FF)>>10;
 		spin_unlock(e->stats_lock);
 	}
-
-	mod_timer(&elist[idx].timer, jiffies + ((HZ<<idx)/4));
+	if (elist[idx].list != NULL)
+		mod_timer(&elist[idx].timer, jiffies + ((HZ<<idx)/4));
 	read_unlock(&est_lock);
 }
 
@@ -173,6 +173,7 @@ int gen_new_estimator(struct gnet_stats_basic *bstats,
 	est->last_packets = bstats->packets;
 	est->avpps = rate_est->pps<<10;
 
+	write_lock_bh(&est_lock);
 	est->next = elist[est->interval].list;
 	if (est->next == NULL) {
 		init_timer(&elist[est->interval].timer);
@@ -181,7 +182,6 @@ int gen_new_estimator(struct gnet_stats_basic *bstats,
 		elist[est->interval].timer.function = est_timer;
 		add_timer(&elist[est->interval].timer);
 	}
-	write_lock_bh(&est_lock);
 	elist[est->interval].list = est;
 	write_unlock_bh(&est_lock);
 	return 0;
@@ -202,7 +202,6 @@ void gen_kill_estimator(struct gnet_stats_basic *bstats,
 	struct gen_estimator *est, **pest;
 
 	for (idx=0; idx <= EST_MAX_INTERVAL; idx++) {
-		int killed = 0;
 		pest = &elist[idx].list;
 		while ((est=*pest) != NULL) {
 			if (est->rate_est != rate_est || est->bstats != bstats) {
@@ -215,10 +214,7 @@ void gen_kill_estimator(struct gnet_stats_basic *bstats,
 			write_unlock_bh(&est_lock);
 
 			kfree(est);
-			killed++;
 		}
-		if (killed && elist[idx].list == NULL)
-			del_timer(&elist[idx].timer);
 	}
 }
 

Re: [NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 251 bytes --]

Patrick McHardy wrote:
> [NET]: gen_estimator: fix locking and timer related bugs
> 


That one still left a race, we could be reinitalizing the timer
while it is still running. This patch additionally makes sure
each timer is only initialized once.


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3980 bytes --]

[NET]: gen_estimator: fix locking and timer related bugs

As noticed by Jarek Poplawski <jarkao2@o2.pl>, the timer removal in
gen_kill_estimator races with the timer function rearming the timer.

Additionally there are a few more related problems that seem to be
relicts from the timer when the estimator was qdisc specific and
could rely on the rtnl or dev->qdisc_lock:

- the check whether the list is empty and a timer needs to be started
  when adding a new estimator doesn't take the lock, so it races
  against concurrent additions, which can result in the timer beeing
  added twice or getting reinitialized after being added.

- the new estimator's next pointer is also set without holding the
  lock, again racing against concurrent additions with possible
  list corruption as a result.

- the timer deletion when killing an estimator is also not under
  the lock and races against timer arming when adding a new estimator.

Fix by holding the lock around the entire list addition and initial
timer arming. Removal is not done explicitly anymore, instead the
timer function only rearms the timer when there are still estimators
present.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit b6a0c468c258d96c6f132fc71ca74225235bc223
tree 6f61004cf4810a4826aa5c7477e4d455ae3a5698
parent 48d8d7ee5dd17c64833e0343ab4ae8ef01cc2648
author Patrick McHardy <kaber@trash.net> Wed, 27 Jun 2007 17:06:02 +0200
committer Patrick McHardy <kaber@trash.net> Wed, 27 Jun 2007 17:24:13 +0200

 net/core/gen_estimator.c |   27 +++++++++++----------------
 1 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
index 17daf4c..88a7805 100644
--- a/net/core/gen_estimator.c
+++ b/net/core/gen_estimator.c
@@ -127,8 +127,8 @@ static void est_timer(unsigned long arg)
 		e->rate_est->pps = (e->avpps+0x1FF)>>10;
 		spin_unlock(e->stats_lock);
 	}
-
-	mod_timer(&elist[idx].timer, jiffies + ((HZ<<idx)/4));
+	if (elist[idx].list != NULL)
+		mod_timer(&elist[idx].timer, jiffies + ((HZ<<idx)/4));
 	read_unlock(&est_lock);
 }
 
@@ -152,6 +152,7 @@ int gen_new_estimator(struct gnet_stats_basic *bstats,
 {
 	struct gen_estimator *est;
 	struct gnet_estimator *parm = RTA_DATA(opt);
+	int idx;
 
 	if (RTA_PAYLOAD(opt) < sizeof(*parm))
 		return -EINVAL;
@@ -163,7 +164,7 @@ int gen_new_estimator(struct gnet_stats_basic *bstats,
 	if (est == NULL)
 		return -ENOBUFS;
 
-	est->interval = parm->interval + 2;
+	est->interval = idx = parm->interval + 2;
 	est->bstats = bstats;
 	est->rate_est = rate_est;
 	est->stats_lock = stats_lock;
@@ -173,16 +174,14 @@ int gen_new_estimator(struct gnet_stats_basic *bstats,
 	est->last_packets = bstats->packets;
 	est->avpps = rate_est->pps<<10;
 
-	est->next = elist[est->interval].list;
-	if (est->next == NULL) {
-		init_timer(&elist[est->interval].timer);
-		elist[est->interval].timer.data = est->interval;
-		elist[est->interval].timer.expires = jiffies + ((HZ<<est->interval)/4);
-		elist[est->interval].timer.function = est_timer;
-		add_timer(&elist[est->interval].timer);
-	}
 	write_lock_bh(&est_lock);
-	elist[est->interval].list = est;
+	if (!elist[idx].timer.function)
+		setup_timer(&elist[idx].timer, est_timer, est->interval);
+	if (elist[est->interval].list == NULL)
+		mod_timer(&elist[idx].timer, jiffies + ((HZ<<idx)/4));
+
+	est->next = elist[idx].list;
+	elist[idx].list = est;
 	write_unlock_bh(&est_lock);
 	return 0;
 }
@@ -202,7 +201,6 @@ void gen_kill_estimator(struct gnet_stats_basic *bstats,
 	struct gen_estimator *est, **pest;
 
 	for (idx=0; idx <= EST_MAX_INTERVAL; idx++) {
-		int killed = 0;
 		pest = &elist[idx].list;
 		while ((est=*pest) != NULL) {
 			if (est->rate_est != rate_est || est->bstats != bstats) {
@@ -215,10 +213,7 @@ void gen_kill_estimator(struct gnet_stats_basic *bstats,
 			write_unlock_bh(&est_lock);
 
 			kfree(est);
-			killed++;
 		}
-		if (killed && elist[idx].list == NULL)
-			del_timer(&elist[idx].timer);
 	}
 }
 

Re: [NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Jarek Poplawski]

On Wed, Jun 27, 2007 at 05:25:45PM +0200, Patrick McHardy wrote:
> Patrick McHardy wrote:
> > [NET]: gen_estimator: fix locking and timer related bugs
> > 
> 
> 
> That one still left a race, we could be reinitalizing the timer
> while it is still running. This patch additionally makes sure
> each timer is only initialized once.
> 

> [NET]: gen_estimator: fix locking and timer related bugs
> 
> As noticed by Jarek Poplawski <jarkao2@o2.pl>, the timer removal in
> gen_kill_estimator races with the timer function rearming the timer.
> 
> Additionally there are a few more related problems that seem to be
> relicts from the timer when the estimator was qdisc specific and

- relicts from the timer when the estimator was qdisc specific and
+ relicts from the time when the estimator was qdisc specific and

> could rely on the rtnl or dev->qdisc_lock:

I've lost some time thinking about this rtnl and checking where
these gen_ functions are used, and how much foolish could be
asking about this here, so, it seems there should be some policy
about commenting required locking in networking - I mean after
reading e.g. sch_generic.c you could wrongly think no comments
means: no locking required. (And probably it would be better/
easier for "the more experienced" to do some supplements, if you
know what I mean...)

> 
> - the check whether the list is empty and a timer needs to be started
>   when adding a new estimator doesn't take the lock, so it races
>   against concurrent additions, which can result in the timer beeing
>   added twice or getting reinitialized after being added.
> 
> - the new estimator's next pointer is also set without holding the
>   lock, again racing against concurrent additions with possible
>   list corruption as a result.
> 
> - the timer deletion when killing an estimator is also not under
>   the lock and races against timer arming when adding a new estimator.
> 
> Fix by holding the lock around the entire list addition and initial
> timer arming. Removal is not done explicitly anymore, instead the
> timer function only rearms the timer when there are still estimators
> present.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> ---
> commit b6a0c468c258d96c6f132fc71ca74225235bc223
> tree 6f61004cf4810a4826aa5c7477e4d455ae3a5698
> parent 48d8d7ee5dd17c64833e0343ab4ae8ef01cc2648
> author Patrick McHardy <kaber@trash.net> Wed, 27 Jun 2007 17:06:02 +0200
> committer Patrick McHardy <kaber@trash.net> Wed, 27 Jun 2007 17:24:13 +0200
> 
>  net/core/gen_estimator.c |   27 +++++++++++----------------
>  1 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
> index 17daf4c..88a7805 100644
> --- a/net/core/gen_estimator.c
> +++ b/net/core/gen_estimator.c
> @@ -127,8 +127,8 @@ static void est_timer(unsigned long arg)
>  		e->rate_est->pps = (e->avpps+0x1FF)>>10;
>  		spin_unlock(e->stats_lock);
>  	}
> -
> -	mod_timer(&elist[idx].timer, jiffies + ((HZ<<idx)/4));
> +	if (elist[idx].list != NULL)
> +		mod_timer(&elist[idx].timer, jiffies + ((HZ<<idx)/4));
>  	read_unlock(&est_lock);
>  }
>  
> @@ -152,6 +152,7 @@ int gen_new_estimator(struct gnet_stats_basic *bstats,
>  {
>  	struct gen_estimator *est;
>  	struct gnet_estimator *parm = RTA_DATA(opt);
> +	int idx;
>  
>  	if (RTA_PAYLOAD(opt) < sizeof(*parm))
>  		return -EINVAL;
> @@ -163,7 +164,7 @@ int gen_new_estimator(struct gnet_stats_basic *bstats,
>  	if (est == NULL)
>  		return -ENOBUFS;
>  
> -	est->interval = parm->interval + 2;
> +	est->interval = idx = parm->interval + 2;
>  	est->bstats = bstats;
>  	est->rate_est = rate_est;
>  	est->stats_lock = stats_lock;
> @@ -173,16 +174,14 @@ int gen_new_estimator(struct gnet_stats_basic *bstats,
>  	est->last_packets = bstats->packets;
>  	est->avpps = rate_est->pps<<10;
>  
> -	est->next = elist[est->interval].list;
> -	if (est->next == NULL) {
> -		init_timer(&elist[est->interval].timer);
> -		elist[est->interval].timer.data = est->interval;
> -		elist[est->interval].timer.expires = jiffies + ((HZ<<est->interval)/4);
> -		elist[est->interval].timer.function = est_timer;
> -		add_timer(&elist[est->interval].timer);
> -	}
>  	write_lock_bh(&est_lock);
> -	elist[est->interval].list = est;
> +	if (!elist[idx].timer.function)

I think, here could be more consistency about "!" or "== NULL".

> +		setup_timer(&elist[idx].timer, est_timer, est->interval);

...and about idx instead of est->interval.

> +	if (elist[est->interval].list == NULL)

idx?

> +		mod_timer(&elist[idx].timer, jiffies + ((HZ<<idx)/4));
> +
> +	est->next = elist[idx].list;
> +	elist[idx].list = est;
>  	write_unlock_bh(&est_lock);
>  	return 0;
>  }
> @@ -202,7 +201,6 @@ void gen_kill_estimator(struct gnet_stats_basic *bstats,
>  	struct gen_estimator *est, **pest;
>  
>  	for (idx=0; idx <= EST_MAX_INTERVAL; idx++) {
> -		int killed = 0;
>  		pest = &elist[idx].list;
>  		while ((est=*pest) != NULL) {
>  			if (est->rate_est != rate_est || est->bstats != bstats) {
> @@ -215,10 +213,7 @@ void gen_kill_estimator(struct gnet_stats_basic *bstats,
>  			write_unlock_bh(&est_lock);
>  
>  			kfree(est);
> -			killed++;
>  		}
> -		if (killed && elist[idx].list == NULL)
> -			del_timer(&elist[idx].timer);

I think this is needed. The old timer could be pending, while
the gen_new_estimator() is run just after this e.g. in
gen_replace_estimator().

>  	}
>  }
>  

Regards,
Jarek P.

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Jarek Poplawski]

On Wed, Jun 27, 2007 at 04:53:48PM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> >On Wed, Jun 27, 2007 at 01:44:08PM +0200, Patrick McHardy wrote:
> >  
> >>>BTW, maybe I look at this too short, but is this del_timer()
> >>>in gen_kill_estimator() enough? I cannot see nothing against
> >>>a timer just running and doing mod_timer() again...
> >>>      
> >>Yes, but nothing bad can happen, the timer will find an empty
> >>list and do nothing. It would make more sense to check for
> >>an empty list before restarting the timer though.

Maybe this time I've looked at this too much, but actually
I think some "badness" is possible: if I'm not missing something
such timer would rearm constantly, so, there could be some
collision when gen_new_estimator() tries to add new timer.

> >>
> >>
> >>Could you send a patch for that?
> >>
> >>    
> >
> >Probably I could, but it's your idea!
> >
> >I look at this just now, and maybe it's enough for asking,
> >but definitely not enough for patch. I'll try to check this
> >more in the evening, so I could send something tomorrow.
> >
> >So if it's not only about kindness, feel free to do it
> >sooner and I've no doubts  - better.
> 
> I can take care of it, no problem.
 
Thanks. I've some technical limitations, so such small changes
take much time if there is a need to get the most up-to-date
kernel and do at least "compile testing". I'm not much about
credits, so I'd prefer to do such things only if it's not very
urgent and could save some signifying amount of your time too.

BTW #2, I hope it's about some new policy, but I cannot see
any #ifdef CONFIG_NET_ESTIMATOR in this sch_htb patch.

Jarek P.

Re: [NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Jarek Poplawski]

On Wed, Jun 27, 2007 at 05:25:45PM +0200, Patrick McHardy wrote:
...
> Additionally there are a few more related problems that seem to be
> relicts from the timer when the estimator was qdisc specific and
> could rely on the rtnl or dev->qdisc_lock:
> 
> - the check whether the list is empty and a timer needs to be started
>   when adding a new estimator doesn't take the lock, so it races
>   against concurrent additions, which can result in the timer beeing
>   added twice or getting reinitialized after being added.
> 
> - the new estimator's next pointer is also set without holding the
>   lock, again racing against concurrent additions with possible
>   list corruption as a result.
> 
> - the timer deletion when killing an estimator is also not under
>   the lock and races against timer arming when adding a new estimator.
> 
> Fix by holding the lock around the entire list addition and initial
> timer arming. Removal is not done explicitly anymore, instead the
> timer function only rearms the timer when there are still estimators
> present.
...
> @@ -202,7 +201,6 @@ void gen_kill_estimator(struct gnet_stats_basic *bstats,
>  	struct gen_estimator *est, **pest;
>  
>  	for (idx=0; idx <= EST_MAX_INTERVAL; idx++) {
> -		int killed = 0;
>  		pest = &elist[idx].list;
>  		while ((est=*pest) != NULL) {

So, maybe this list walking here needs some locking too?

Jarek P.

Re: [NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Jarek Poplawski]

On Thu, Jun 28, 2007 at 08:54:48AM +0200, Jarek Poplawski wrote:
...
> > @@ -215,10 +213,7 @@ void gen_kill_estimator(struct gnet_stats_basic *bstats,
> >  			write_unlock_bh(&est_lock);
> >  
> >  			kfree(est);
> > -			killed++;
> >  		}
> > -		if (killed && elist[idx].list == NULL)
> > -			del_timer(&elist[idx].timer);
> 
> I think this is needed. The old timer could be pending, while
> the gen_new_estimator() is run just after this e.g. in
> gen_replace_estimator().

Sorry! I've forgotten there is mod_timer now, so, it's OK!

Jarek P.

Re: [NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Patrick McHardy]

Jarek Poplawski wrote:
>> @@ -202,7 +201,6 @@ void gen_kill_estimator(struct gnet_stats_basic *bstats,
>>  	struct gen_estimator *est, **pest;
>>  
>>  	for (idx=0; idx <= EST_MAX_INTERVAL; idx++) {
>> -		int killed = 0;
>>  		pest = &elist[idx].list;
>>  		while ((est=*pest) != NULL) {
>
> So, maybe this list walking here needs some locking too?

It depends on whether estimators should be able to rely on
the rtnl in the future or be completely responsible for their
own locking. My patch yesterday was made under the assumption
that they shouldn't rely on external locking, which seemed to
be the right thing for a "generic" implementation. OTOH its
still specific to networking, so relying on the rtnl doesn't
sound too unreasonable too. I'm beginning to thing I made
the wrong choice with my patch.

I'm busy right now, would you mind looking into a patch that
only deals with the timer races, but still relies on the
rtnl?

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Patrick McHardy]

Jarek Poplawski wrote:
> BTW #2, I hope it's about some new policy, but I cannot see
> any #ifdef CONFIG_NET_ESTIMATOR in this sch_htb patch.

One of my previous patches for 2.6.23 killed that option,
the code was always compiled in anyways.

Re: [NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Patrick McHardy]

Jarek Poplawski wrote:
> On Thu, Jun 28, 2007 at 02:23:36PM +0200, Patrick McHardy wrote:
> 
>>Jarek Poplawski wrote:
>>
>>>>@@ -202,7 +201,6 @@ void gen_kill_estimator(struct gnet_stats_basic 
>>>>*bstats,
>>>>	struct gen_estimator *est, **pest;
>>>>
>>>>	for (idx=0; idx <= EST_MAX_INTERVAL; idx++) {
>>>>-		int killed = 0;
>>>>		pest = &elist[idx].list;
>>>>		while ((est=*pest) != NULL) {
>>>
>>>So, maybe this list walking here needs some locking too?
>>
>>It depends on whether estimators should be able to rely on
>>the rtnl in the future or be completely responsible for their
>>own locking. My patch yesterday was made under the assumption
>>that they shouldn't rely on external locking, which seemed to
>>be the right thing for a "generic" implementation. OTOH its
>>still specific to networking, so relying on the rtnl doesn't
>>sound too unreasonable too. I'm beginning to thing I made
>>the wrong choice with my patch.
>>
>>I'm busy right now, would you mind looking into a patch that
>>only deals with the timer races, but still relies on the
>>rtnl?
> 
> 
> In that case this patch looks OK & enough.


Its overkill in that case. The concurrent additions and removals
can't happen.

Re: [NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Jarek Poplawski]

On Thu, Jun 28, 2007 at 02:23:36PM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> >>@@ -202,7 +201,6 @@ void gen_kill_estimator(struct gnet_stats_basic 
> >>*bstats,
> >> 	struct gen_estimator *est, **pest;
> >> 
> >> 	for (idx=0; idx <= EST_MAX_INTERVAL; idx++) {
> >>-		int killed = 0;
> >> 		pest = &elist[idx].list;
> >> 		while ((est=*pest) != NULL) {
> >
> >So, maybe this list walking here needs some locking too?
> 
> It depends on whether estimators should be able to rely on
> the rtnl in the future or be completely responsible for their
> own locking. My patch yesterday was made under the assumption
> that they shouldn't rely on external locking, which seemed to
> be the right thing for a "generic" implementation. OTOH its
> still specific to networking, so relying on the rtnl doesn't
> sound too unreasonable too. I'm beginning to thing I made
> the wrong choice with my patch.
> 
> I'm busy right now, would you mind looking into a patch that
> only deals with the timer races, but still relies on the
> rtnl?

In that case this patch looks OK & enough.

My earlier proposals are only of cosmetical value.

Jarek P.

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Patrick McHardy]

Jarek Poplawski wrote:
> On Thu, Jun 28, 2007 at 02:24:55PM +0200, Patrick McHardy wrote:
> 
>>Jarek Poplawski wrote:
>>
>>>BTW #2, I hope it's about some new policy, but I cannot see
>>>any #ifdef CONFIG_NET_ESTIMATOR in this sch_htb patch.
>>
>>One of my previous patches for 2.6.23 killed that option,
>>the code was always compiled in anyways.
>>
> 
> 
> Maybe I look at something else but I cannot find this patch,
> at least here:
> 
> http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.23.git;a=tree


No, its not in Dave's tree yet.

Re: [Bugme-new] [Bug 8668] New: HTB Deadlock

[Jarek Poplawski]

On Thu, Jun 28, 2007 at 02:24:55PM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> >BTW #2, I hope it's about some new policy, but I cannot see
> >any #ifdef CONFIG_NET_ESTIMATOR in this sch_htb patch.
> 
> One of my previous patches for 2.6.23 killed that option,
> the code was always compiled in anyways.
> 

Maybe I look at something else but I cannot find this patch,
at least here:

http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.23.git;a=tree

Jarek P.

Re: [NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Jarek Poplawski]

On Thu, Jun 28, 2007 at 02:55:51PM +0200, Patrick McHardy wrote:
...
> Its overkill in that case. The concurrent additions and removals
> can't happen.
> 

Then the changelog needs one more change. Plus, maybe - btw,
1 line about this at the beginning of the file?

Jarek P.

Re: [NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Jarek Poplawski]

On Thu, Jun 28, 2007 at 02:55:51PM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> > On Thu, Jun 28, 2007 at 02:23:36PM +0200, Patrick McHardy wrote:
> > 
> >>Jarek Poplawski wrote:
> >>
> >>>>@@ -202,7 +201,6 @@ void gen_kill_estimator(struct gnet_stats_basic 
> >>>>*bstats,
> >>>>	struct gen_estimator *est, **pest;
> >>>>
> >>>>	for (idx=0; idx <= EST_MAX_INTERVAL; idx++) {
> >>>>-		int killed = 0;
> >>>>		pest = &elist[idx].list;
> >>>>		while ((est=*pest) != NULL) {
> >>>
...
> Its overkill in that case. The concurrent additions and removals
> can't happen.

BTW, if we talk about overkills: is there any reason to do these
for & while until the end? I can't see why anybody should add the
same *bstats & *rate_est more than once (or max twice if we let
to add, change & remove them independently). With a large number
of classes this could matter.

Regards,
Jarek P.

Re: [NET]: gen_estimator: fix locking and timer related bugs [Re: [Bugme-new] [Bug 8668] New: HTB Deadlock]

[Jarek Poplawski]

On Fri, Jun 29, 2007 at 09:02:41AM +0200, Jarek Poplawski wrote:
...
> same *bstats & *rate_est more than once (or max twice if we let
> to add, change & remove them independently).

...but this doesn't look sensible at all!

So, maybe, if we would need something counted with two intervals...
But, nobody seems to use such possibility, anyway.

Jarek P.