Re: circular locking, mirred, 2.6.24.2

[jamal <hadi@cyberus.ca>]

Jarek,

On Thu, 2008-06-03 at 22:40 +0100, Jarek Poplawski wrote:

> But currently lockdep doesn't know dev->queue_lock could mean eth or lo.
> It sees one class of devices using one lock. 

yikes ;-> I missed the implication when you mentioned it first - thanks
for the enlightnment. IMO, it needs to be per netdevice not global; so i
contend this is the problem i.e those warnings are false.

> We can let it know (e.g.
> dev->_xmit_lock is different according to dev->type), but it wasn't
> necessary. I hope it will suffice here if lockdep knows more about ifb,
> but similar problem could theoretically happen with other devs.

I think the condition needs to be met for all netdevices, not just ifb.
Instead of annotating each netdevice type separately, could you not use
a different annotation in the generic netdev registration code of the
form dev_%s->ingress/queue_lock ? (where %s is dev->name)

> Sorry, should be:
>  dev_eth0->queue_lock, and thread3 has dev_eth0->queue_lock and wants

indeed - i understood when you said that; it doesnt invalidate what i
said.

> > thread3 can not happen because we dont allow it (the other 2 are not
> > contentious).
> 
> Could you explain why? 

Because we never redirect to ingress (it is in the todo as mentioned as
described at the top of the source file). You can redirect from ingress
or egress but only to egress (to sockets and ingress are going to happen
at some future point).

> Yes, but it seems such redirection between two eths like above mentioned
> is legal?

Indeed it is. I think it is clear to me now the issues seem to be the
namespace of what lockdep - it should be per-device and NOT global.
What do you think?

cheers,
jamal