sch_generic: warning: the comparison will always evaluate as ‘true’ for the address of ‘noop_qdisc’ will never be NULL

With:

gcc (GCC) 4.6.1

I noticed the following warning appearing in my build:

net/sched/sch_generic.c: In function ‘dev_graft_qdisc’:
net/sched/sch_generic.c:678:2: warning: the comparison will always
evaluate as ‘true’ for the address of ‘noop_qdisc’ will never be NULL
[-Waddress]

The code in question runs:


        /* ... and graft new one */
        if (qdisc == NULL)
                qdisc = &noop_qdisc;
        dev_queue->qdisc_sleeping = qdisc;
        rcu_assign_pointer(dev_queue->qdisc, &noop_qdisc);

where rcu_assign_pointer has a null check that does not apply to
noop_qdisc, which will never be null.

My question is, should that really be assigning &noop_qdisc in there?
It seems odd to assign &noop_qdisc to qdisc only if qdisc is null, and
then unconditionally assign &noop_qdisc into dev_queue->qdisc.

Should the line be:

        rcu_assign_pointer(dev_queue->qdisc, qdisc);

instead?

Just curious,

-- 
Kevin Winchester

Le dimanche 14 août 2011 à 10:44 -0300, Kevin Winchester a écrit :

With:

gcc (GCC) 4.6.1

I noticed the following warning appearing in my build:

net/sched/sch_generic.c: In function ‘dev_graft_qdisc’:
net/sched/sch_generic.c:678:2: warning: the comparison will always
evaluate as ‘true’ for the address of ‘noop_qdisc’ will never be NULL
[-Waddress]

The code in question runs:


        /* ... and graft new one */
        if (qdisc == NULL)
                qdisc = &noop_qdisc;
        dev_queue->qdisc_sleeping = qdisc;
        rcu_assign_pointer(dev_queue->qdisc, &noop_qdisc);

where rcu_assign_pointer has a null check that does not apply to
noop_qdisc, which will never be null.

gcc is a bit stupid here. Of course we know &noop_qdisc cannot be NULL.

My question is, should that really be assigning &noop_qdisc in there?
It seems odd to assign &noop_qdisc to qdisc only if qdisc is null, and
then unconditionally assign &noop_qdisc into dev_queue->qdisc.

Should the line be:

        rcu_assign_pointer(dev_queue->qdisc, qdisc);

instead?

Just curious,

This was already taken into account, the trick is to make rcu_assign()
not trying to be smart, and use RCU_INIT_POINTER() in places we want to
assign NULL pointers.

So one patch is carried by Paul McKenney (RCU maintainer) for next
kernel, and other one in net-next :

http://git2.kernel.org/?p=linux/kernel/git/paulmck/linux-2.6-rcu.git;a=commitdiff;h=a7590ddfc2c855e75111ef18147a86578fe136e4

http://git2.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commitdiff;h=a9b3cd7f323b2e57593e7215362a7b02fc933e3a