[PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Ismail Donmez]

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

Hi,

With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE 
changed behaviour so it returns false when the node is empty as expected. 
Hence Herbert's fix for sched_htb.c should be reverted.

[1] 
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=10fd48f2376db52f08bf0420d2c4f580e39269e1;hp=9817064b68fef7e4580c6df1ea597e106b9ff88b

Regards,
ismail

-- 
They that can give up essential liberty to obtain a little temporary safety 
deserve neither liberty nor safety.
-- Benjamin Franklin

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

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 6c058e3..1f1360e 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -391,7 +391,7 @@ static inline void htb_add_class_to_row(
 /* If this triggers, it is a bug in this code, but it need not be fatal */
 static void htb_safe_rb_erase(struct rb_node *rb, struct rb_root *root)
 {
-	if (!RB_EMPTY_NODE(rb)) {
+	if (RB_EMPTY_NODE(rb)) {
 		WARN_ON(1);
 	} else {
 		rb_erase(rb, root);

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Ismail Donmez]

On Saturday 30 September 2006 22:23, you wrote:
> Hi,
>
> With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE
> changed behaviour so it returns false when the node is empty as expected.
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
make it : so it returns true when the node is empty as expected

/ismail
-- 
They that can give up essential liberty to obtain a little temporary safety 
deserve neither liberty nor safety.
-- Benjamin Franklin

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Herbert Xu]

On Sat, Sep 30, 2006 at 10:23:46PM +0300, Ismail Donmez wrote:
> 
> With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE 
> changed behaviour so it returns false when the node is empty as expected. 
> Hence Herbert's fix for sched_htb.c should be reverted.

I've fixed sched_htb.c? That's news to me :)

I fully agree with your patch though.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Ismail Donmez]

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

On Sunday 01 October 2006 04:52, Herbert Xu wrote:
> On Sat, Sep 30, 2006 at 10:23:46PM +0300, Ismail Donmez wrote:
> > With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE
> > changed behaviour so it returns false when the node is empty as expected.
> > Hence Herbert's fix for sched_htb.c should be reverted.
>
> I've fixed sched_htb.c? That's news to me :)
>
> I fully agree with your patch though.

Oh it was Patrick McHardy <kaber@trash.net>, sorry. Patch again with a 
Signed-off line too this time.

Hi,

With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE 
changed behaviour so it returns false when the node is empty as expected. 
Hence Patrick McHardy's fix for sched_htb.c should be reverted.

[1] 
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=10fd48f2376db52f08bf0420d2c4f580e39269e1;hp=9817064b68fef7e4580c6df1ea597e106b9ff88b

Signed-off-by: Ismail Donmez <ismail@pardus.org.tr>

Regards,
ismail

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

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 6c058e3..1f1360e 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -391,7 +391,7 @@ static inline void htb_add_class_to_row(
 /* If this triggers, it is a bug in this code, but it need not be fatal */
 static void htb_safe_rb_erase(struct rb_node *rb, struct rb_root *root)
 {
-	if (!RB_EMPTY_NODE(rb)) {
+	if (RB_EMPTY_NODE(rb)) {
 		WARN_ON(1);
 	} else {
 		rb_erase(rb, root);

[PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Ismail Donmez]

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

This time with correct description too, *sigh*

With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE 
changed behaviour so it returns true when the node is empty as expected. 
Hence Patrick McHardy's fix for sched_htb.c should be reverted.

[1] 
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=10fd48f2376db52f08bf0420d2c4f580e39269e1;hp=9817064b68fef7e4580c6df1ea597e106b9ff88b

Signed-off-by: Ismail Donmez <ismail@pardus.org.tr>

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

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 6c058e3..1f1360e 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -391,7 +391,7 @@ static inline void htb_add_class_to_row(
 /* If this triggers, it is a bug in this code, but it need not be fatal */
 static void htb_safe_rb_erase(struct rb_node *rb, struct rb_root *root)
 {
-	if (!RB_EMPTY_NODE(rb)) {
+	if (RB_EMPTY_NODE(rb)) {
 		WARN_ON(1);
 	} else {
 		rb_erase(rb, root);

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Jarek Poplawski]

On 30-09-2006 21:23, Ismail Donmez wrote:
> Hi,
> 
> With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE 
> changed behaviour so it returns false when the node is empty as expected. 
...
> -	if (!RB_EMPTY_NODE(rb)) {
> +	if (RB_EMPTY_NODE(rb)) {

Maybe you have some kind of agreement with Jens Axboe but I
can't understand current way of kernel cooperation:
he changes some global behavior to the opposite and fixes
his code in three places but can't fix it in the fourth place
where it's used? Isn't it both trivial and automatic kind
of patch?

Second question is this title alarming enough?: 
"[PATCH] rbtree: fixed reversed RB_EMPTY_NODE and rb_next/prev"

Maybe:
[PATCH] rbtree: reversed RB_EMPTY_NODE and fixed rb_next/prev

but shouldn't there be some special [XYZ!] keyword used for
such severe situations? 

Jarek P.  

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Jarek Poplawski]

On Mon, Oct 02, 2006 at 12:24:47PM +0200, Jarek Poplawski wrote:
> On 30-09-2006 21:23, Ismail Donmez wrote:
> > Hi,
> > 
> > With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE 
> > changed behaviour so it returns false when the node is empty as expected. 
> ...
> > -	if (!RB_EMPTY_NODE(rb)) {
> > +	if (RB_EMPTY_NODE(rb)) {
> 
> Maybe you have some kind of agreement with Jens Axboe but I
> can't understand current way of kernel cooperation:
> he changes some global behavior to the opposite and fixes
> his code in three places but can't fix it in the fourth place
...

But I see it's not so bad and net_sched isn't the last!
It looks deadline-iosched.c and one place in as-iosched.c
(~ 466 line) where probably also forgotten. 

Another question - is there any change planned?
If not why in rbtree.c is:

+ if (rb_parent(node) == node)

instead of:

+ if (RB_EMPTY_NODE(rb))

Jarek P.

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Jarek Poplawski]

On Mon, Oct 02, 2006 at 01:15:55PM +0200, Jarek Poplawski wrote:
...
> instead of:
> 
> + if (RB_EMPTY_NODE(rb))

should be:

+ if (RB_EMPTY_NODE(node))

Jarek P.

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Ismail Donmez]

02 Eki 2006 Pts 13:24 tarihinde, Jarek Poplawski şunları yazmıştı: 
> On 30-09-2006 21:23, Ismail Donmez wrote:
> > Hi,
> >
> > With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE
> > changed behaviour so it returns false when the node is empty as expected.
>
> ...
>
> > -	if (!RB_EMPTY_NODE(rb)) {
> > +	if (RB_EMPTY_NODE(rb)) {
>
> Maybe you have some kind of agreement with Jens Axboe but I
> can't understand current way of kernel cooperation:
> he changes some global behavior to the opposite and fixes
> his code in three places but can't fix it in the fourth place
> where it's used? Isn't it both trivial and automatic kind
> of patch?

I don't know if Jens will going to fix other occurrences but the sch_htb.c fix 
was recent and Jens'  commit got my attention hence the patch.

Regards,
ismail

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Jarek Poplawski]

On Mon, Oct 02, 2006 at 04:49:38PM +0300, Ismail Donmez wrote:
> 02 Eki 2006 Pts 13:24 tarihinde, Jarek Poplawski şunlar?? yazm??şt??: 
> > On 30-09-2006 21:23, Ismail Donmez wrote:
> > > Hi,
> > >
> > > With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE
> > > changed behaviour so it returns false when the node is empty as expected.
> >
> > ...
> >
> > > -	if (!RB_EMPTY_NODE(rb)) {
> > > +	if (RB_EMPTY_NODE(rb)) {
> >
> > Maybe you have some kind of agreement with Jens Axboe but I
> > can't understand current way of kernel cooperation:
> > he changes some global behavior to the opposite and fixes
> > his code in three places but can't fix it in the fourth place
> > where it's used? Isn't it both trivial and automatic kind
> > of patch?
> 
> I don't know if Jens will going to fix other occurrences but the sch_htb.c fix 
> was recent and Jens'  commit got my attention hence the patch.

As a matter of fact this kind of cooperation has also many pluses:
developers have to be more vigilant, read about other's work,
more people are engaged and last but not least there is this
infallible akpm reminding subsystem...

Best regards,

Jarek P. 

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Jens Axboe]

On Mon, Oct 02 2006, Jarek Poplawski wrote:
> On Mon, Oct 02, 2006 at 12:24:47PM +0200, Jarek Poplawski wrote:
> > On 30-09-2006 21:23, Ismail Donmez wrote:
> > > Hi,
> > > 
> > > With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE 
> > > changed behaviour so it returns false when the node is empty as expected. 
> > ...
> > > -	if (!RB_EMPTY_NODE(rb)) {
> > > +	if (RB_EMPTY_NODE(rb)) {
> > 
> > Maybe you have some kind of agreement with Jens Axboe but I
> > can't understand current way of kernel cooperation:
> > he changes some global behavior to the opposite and fixes
> > his code in three places but can't fix it in the fourth place
> ...
> 
> But I see it's not so bad and net_sched isn't the last!
> It looks deadline-iosched.c and one place in as-iosched.c
> (~ 466 line) where probably also forgotten. 

I don't see any missed changes.

> Another question - is there any change planned?
> If not why in rbtree.c is:
> 
> + if (rb_parent(node) == node)
> 
> instead of:
> 
> + if (RB_EMPTY_NODE(rb))

RB_EMPTY_NODE was (re)-introduced after that change, and it never
propagated. Changing it now would be fine and easier to read.

-- 
Jens Axboe

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Jens Axboe]

On Mon, Oct 02 2006, Ismail Donmez wrote:
> 02 Eki 2006 Pts 13:24 tarihinde, Jarek Poplawski ??unlar?? yazm????t??: 
> > On 30-09-2006 21:23, Ismail Donmez wrote:
> > > Hi,
> > >
> > > With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE
> > > changed behaviour so it returns false when the node is empty as expected.
> >
> > ...
> >
> > > -	if (!RB_EMPTY_NODE(rb)) {
> > > +	if (RB_EMPTY_NODE(rb)) {
> >
> > Maybe you have some kind of agreement with Jens Axboe but I
> > can't understand current way of kernel cooperation:
> > he changes some global behavior to the opposite and fixes
> > his code in three places but can't fix it in the fourth place
> > where it's used? Isn't it both trivial and automatic kind
> > of patch?
> 
> I don't know if Jens will going to fix other occurrences but the
> sch_htb.c fix was recent and Jens'  commit got my attention hence the
> patch.

Send me a patch and I'll review / integrate it. Most of the rbtree
mixups stem from the fact that the first color change was incomplete.

-- 
Jens Axboe

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Jarek Poplawski]

On Tue, Oct 03, 2006 at 12:28:32PM +0200, Jens Axboe wrote:
...
> I don't see any missed changes.

And you are right!
I've checked the link from the first message of this thread
and now I see it's not current enough. 
I'm sorry for this false alarm. 

Jarek P.

[PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Ismail Donmez]

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

With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE 
changed behaviour so it returns true when the node is empty as expected. 
Hence Patrick McHardy's fix for sched_htb.c should be reverted.

Signed-off-by: Ismail Donmez <ismail@pardus.org.tr>

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

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 6c058e3..1f1360e 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -391,7 +391,7 @@ static inline void htb_add_class_to_row(
 /* If this triggers, it is a bug in this code, but it need not be fatal */
 static void htb_safe_rb_erase(struct rb_node *rb, struct rb_root *root)
 {
-	if (!RB_EMPTY_NODE(rb)) {
+	if (RB_EMPTY_NODE(rb)) {
 		WARN_ON(1);
 	} else {
 		rb_erase(rb, root);

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[Patrick McHardy]

Ismail Donmez wrote:
> With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE 
> changed behaviour so it returns true when the node is empty as expected. 
> Hence Patrick McHardy's fix for sched_htb.c should be reverted.
> 
> Signed-off-by: Ismail Donmez <ismail@pardus.org.tr>

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

Re: [PATCH] Revert [NET_SCHED]: HTB: fix incorrect use of RB_EMPTY_NODE

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue, 03 Oct 2006 20:05:33 +0200

> Ismail Donmez wrote:
> > With commit 10fd48f2376db52f08bf0420d2c4f580e39269e1 [1] ,  RB_EMPTY_NODE 
> > changed behaviour so it returns true when the node is empty as expected. 
> > Hence Patrick McHardy's fix for sched_htb.c should be reverted.
> > 
> > Signed-off-by: Ismail Donmez <ismail@pardus.org.tr>
> 
> ACKed-by: Patrick McHardy <kaber@trash.net>

Applied, thanks everyone.