Re: [PATCH 2/3] [VLAN]: Update iif when receiving via VLAN device

[Jamal Hadi Salim <hadi@cyberus.ca>]

On Sun, 2006-09-07 at 15:33 +0200, Thomas Graf wrote:
> * Jamal Hadi Salim <hadi@cyberus.ca> 2006-07-09 08:52
[..]
> > Another simpler approach is to check for recursion right in 
> 
> It's very interesting that you change from "there is no easy way"
> to "another simpler approach..." in just one posting :-)
> 

I said there was no easy way of detecting - which is different from
preventing. Both approaches i showed were things i used (as far back as
last year) but did not find palatable to submit.
Hopefully we dont go into a tangent on this.

> > if (q->enqueue) {
> > 		if (!spin_trylock(&dev->queue_lock)) {
> > 			printk("yikes recursed device %s\n",dev->name);
> >                         goto tx_recursion_detected;
> >                 }
> 
> That's not gonna work, dev->queue_lock may be held legimitately
> by someone else than an underlying dev_queue_xmit() call.
> 

If there is a legitimate reason then it wont work. I cant think of one
though.

> > [BTW, notice how i used code to describe my view above ;->]
> 
> I'm very proud of you.
> 

Thank you thank you

> > Again, I was hoping that Herbert's stuff may change this view (and
> > therefore give it more legitimacy) - but if the above can be accepted
> > then we can forget touching mirred.
> 
> You need this:
> 
> if (test_and_set_bit(__LINK_STATE_ENQUEUEING, &dev->state))
> 	goto tx_recursion_detected;
> 
> spin_lock(queue_lock);
> enqueue(...)
> qdisc_run()
> spin_unlock(queue_lock);
> 
> clear_bit(__LINK_TATE_ENQUEUEING, &dev->state);
> 
> Unfortunately we'd still need __LINK_STATE_QDISC_RUNNING due
> to net_tx_action().
> 

This is also another approach that would work. If you think its simpler
go ahead and shoot a patch.

> > By "mirred deadlock" i think you mean the deadlock that happens in
> > dev_queue_xmit()? 
> 
> I meant the deadlock within mirred but the deadlock on queue_lock
> is happening first anyway.
> 

if you can stop things at the queue you wont have to worry about mirred.

> > What is still not clear above?
> 
> Frankly, I have no idea which deadlocks are intentional, what ideas
> you have about ingress->egress, etc because it is not documented.
> The list is very long. 

Ask one question at a time and i will answer. Some of the things we
discussed have no place to be commented-on in the code. But i think what
is obvious is:

A->*->A is a no-no.
And in some cases it is fine to let the user just fsck themselves
because then they will understand it is a bad idea [1] when shit
happens. OTOH, if there was a KISS way of doing it (as in the ifb case,
why not).

> I'm not going to waste time trying to work
> out a patch that will then not suit the ideas in your mind and on
> your notes. You're maintaing this code, no?

Yes, of course otherwise i wouldnt bother to comment on any patches.

cheers,
jamal

[1] there was a long discussion a few years back when i was still in
lkml on someone trying to redirect (i think) /dev/null -> /dev/mem
(dont hold me accountable if those are not the right devices, just
absorb the moral of this instead). The consensus was it would be very
difficult to do without making a lot of changes and if someone wishes to
do that they deserved what they are getting.