* sch_generic: warning: the comparison will always evaluate as ‘true’ for the address of ‘noop_qdisc’ will never be NULL
@ 2011-08-14 13:44 Kevin Winchester
2011-08-14 17:39 ` Eric Dumazet
0 siblings, 1 reply; 2+ messages in thread
From: Kevin Winchester @ 2011-08-14 13:44 UTC (permalink / raw)
To: hadi, davem; +Cc: netdev, LKML
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: sch_generic: warning: the comparison will always evaluate as ‘true’ for the address of ‘noop_qdisc’ will never be NULL
2011-08-14 13:44 sch_generic: warning: the comparison will always evaluate as ‘true’ for the address of ‘noop_qdisc’ will never be NULL Kevin Winchester
@ 2011-08-14 17:39 ` Eric Dumazet
0 siblings, 0 replies; 2+ messages in thread
From: Eric Dumazet @ 2011-08-14 17:39 UTC (permalink / raw)
To: Kevin Winchester; +Cc: hadi, davem, netdev, LKML, Paul E. McKenney
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2011-08-14 17:40 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-14 13:44 sch_generic: warning: the comparison will always evaluate as ‘true’ for the address of ‘noop_qdisc’ will never be NULL Kevin Winchester
2011-08-14 17:39 ` Eric Dumazet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.