On Sun, 2006-09-07 at 08:52 -0400, Jamal Hadi Salim wrote:
Ok, didnt complete my thought there ..
I dont like it, like i said, because it adds one more check
in the first path. I was contemplating at some point even not doing the
.. not doing the check for device up and just shove the
* Jamal Hadi Salim [EMAIL PROTECTED] 2006-07-09 08:52
On Sun, 2006-09-07 at 01:46 +0200, Thomas Graf wrote:
* Jamal Hadi Salim [EMAIL PROTECTED] 2006-07-08 10:14
[..]
There is no easy way to detect such a deadlock. I think it reduces to
the same case as eth0-eth0, i.e:
It's very
On Sun, 2006-09-07 at 15:33 +0200, Thomas Graf wrote:
* Jamal Hadi Salim [EMAIL PROTECTED] 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
* Jamal Hadi Salim [EMAIL PROTECTED] 2006-07-09 10:03
On Sun, 2006-09-07 at 15:33 +0200, Thomas Graf wrote:
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
On Sun, 2006-09-07 at 16:19 +0200, Thomas Graf wrote:
* Jamal Hadi Salim [EMAIL PROTECTED] 2006-07-09 10:03
On Sun, 2006-09-07 at 15:33 +0200, Thomas Graf wrote:
That's not gonna work, dev-queue_lock may be held legimitately
by someone else than an underlying dev_queue_xmit() call.
* Jamal Hadi Salim [EMAIL PROTECTED] 2006-07-09 11:00
If you mean that the device will also try to grab the qlock there, then
that is fine still for the serialization. It all starts at
dev_queue_xmit.
Look at where dev-queue_lock is taken, whenever a qdisc or
filter is added, modified or
* jamal [EMAIL PROTECTED] 2006-07-01 09:35
On Sat, 2006-01-07 at 13:28 +0200, Thomas Graf wrote:
Please enlighten me, I may be making a wrong assumption again.
1) tc_verd is reset to 0 after dq in ri_tasklet()
2) TC_AT is set to AT_EGRESS in dev_queue_xmit()
3) TC_FROM being derived
On Sat, 2006-08-07 at 10:14 -0400, Jamal Hadi Salim wrote:
I have a dated patch to mirred (may not apply cleanly)
Sorry forgot to attach the patch. Attached for real this time;-
that i believe
will fix this specific one. Try to see if it also fixes this case you
have.
I meant i know this
* Thomas Graf [EMAIL PROTECTED] 2006-07-09 01:46
__LINK_STATE_QDISC_RUNNING will prevent an eventual tx deadlock,
it will not prevent the mirred deadlock.
BTW, it will also not protect you from deadlocking on
dev-queue_lock while enqueueing.
-
To unsubscribe from this list: send the line
* jamal [EMAIL PROTECTED] 2006-06-30 22:59
On Sat, 2006-01-07 at 01:45 +0200, Thomas Graf wrote:
Fact is that the from verdict is set to a meaningful value again at
dev_queue_xmit() or ing_filter() so ifb_xmit() only sees valid values.
Ok, Thomas this is one of those places where we end
* jamal [EMAIL PROTECTED] 2006-06-30 22:23
I am not certain i understood then: Are we in the mode where the
refcount is not needed because chances are small that a device will
disappear? It seems to me after all this trouble that it may not be so
bad to refcount (I guess i meant refcount the
On Sat, 2006-01-07 at 13:28 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-30 22:59
Please enlighten me, I may be making a wrong assumption again.
1) tc_verd is reset to 0 after dq in ri_tasklet()
2) TC_AT is set to AT_EGRESS in dev_queue_xmit()
3) TC_FROM being derived from
On Sat, 2006-01-07 at 13:51 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-30 22:23
I am not certain i understood then: Are we in the mode where the
refcount is not needed because chances are small that a device will
disappear? It seems to me after all this trouble that it may
* jamal [EMAIL PROTECTED] 2006-06-29 21:11
Heres what it would look at ingress:
step 0: coming from wire via eth0,
dev=eth0, input_dev=eth0
step 1: redirect to ifb0, leaving redirect
dev=ifb0, input_dev=eth0
step 2: leaving ifb0, coming back to ingress side of stack
dev= eth0,
Thomas Graf a écrit :
* Nicolas Dichtel [EMAIL PROTECTED] 2006-06-30 15:16
That creates a nice loop on ingress. Upon reentering the
stack with skb-dev set to eth0 again we'll go through the
same ingress filters as the first time and we'll hit ifb0
again over and over. Are you suggesting
On Fri, 2006-30-06 at 15:08 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-29 21:11
Heres what it would look at ingress:
step 0: coming from wire via eth0,
dev=eth0, input_dev=eth0
step 1: redirect to ifb0, leaving redirect
dev=ifb0, input_dev=eth0
step 2:
On Fri, 2006-30-06 at 16:15 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-30 09:57
On Fri, 2006-30-06 at 15:08 +0200, Thomas Graf wrote:
That creates a nice loop on ingress. Upon reentering the
stack with skb-dev set to eth0 again we'll go through the
same ingress filters
* jamal [EMAIL PROTECTED] 2006-06-30 10:35
On Fri, 2006-30-06 at 16:15 +0200, Thomas Graf wrote:
eth0::tcf_mirred() skb-dev = ifb0, input_dev = eth0
ifb0::tcf_mirred() skb-dev = ifb1, input_dev = ifb0
ifb1::ifb_xmit() skb-dev = ifb0, input_dev = ifb1, set ncls bit
Nicolas Dichtel wrote:
Bit 8 of skb-tc_verd is set by IFB, so packet isn't reclassify.
This bit avoid the loop.
It would, if something would actually set it.
~/src/kernel/linux-2.6$ grep NCLS -r net/
net/core/dev.c: if (skb-tc_verd TC_NCLS) {
net/core/dev.c: skb-tc_verd =
Patrick McHardy wrote:
Nicolas Dichtel wrote:
Bit 8 of skb-tc_verd is set by IFB, so packet isn't reclassify.
This bit avoid the loop.
It would, if something would actually set it.
~/src/kernel/linux-2.6$ grep NCLS -r net/
net/core/dev.c: if (skb-tc_verd TC_NCLS) {
net/core/dev.c:
On Fri, 2006-30-06 at 18:32 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-30 10:35
Did you actually try to run this before you reached this conclusion?
I did, fortunately some other bug prevents this from happening,
packets are simply dropped somewhere.
It is not a bug,
Thomas Graf wrote:
* Thomas Graf [EMAIL PROTECTED] 2006-06-30 18:32
Anyways, I give up. Last time I've been running after you trying
to fix the many bugs you leave behind. Ever noticed that whenever
you add some new code it's someone else following up with tons of
small bugfix patches having a
Thomas Graf wrote:
* Thomas Graf [EMAIL PROTECTED] 2006-06-30 18:32
Anyways, I give up. Last time I've been running after you trying
to fix the many bugs you leave behind. Ever noticed that whenever
you add some new code it's someone else following up with tons of
small bugfix patches having a
On Fri, 2006-30-06 at 19:18 +0200, Patrick McHardy wrote:
Thomas Graf wrote:
* Thomas Graf [EMAIL PROTECTED] 2006-06-30 18:32
Anyways, I give up. Last time I've been running after you trying
to fix the many bugs you leave behind. Ever noticed that whenever
you add some new code it's
* jamal [EMAIL PROTECTED] 2006-06-30 13:19
On Fri, 2006-30-06 at 18:32 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-30 10:35
Did you actually try to run this before you reached this conclusion?
I did, fortunately some other bug prevents this from happening,
packets
jamal wrote:
On Fri, 2006-30-06 at 19:18 +0200, Patrick McHardy wrote:
Thomas Graf wrote:
* Thomas Graf [EMAIL PROTECTED] 2006-06-30 18:32
Anyways, I give up. Last time I've been running after you trying
to fix the many bugs you leave behind. Ever noticed that whenever
you add some new code
On Fri, 2006-30-06 at 19:42 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-30 13:32
Thomas makes the claim, this can be achieved only by using an ifindex.
And i havent been able to see how. I have a small performance problem if
i just use ifindex. Using ifindex will eventually
On Fri, 2006-30-06 at 19:42 +0200, Patrick McHardy wrote:
jamal wrote:
Thomas makes the claim, this can be achieved only by using an ifindex.
And i havent been able to see how. I have a small performance problem if
i just use ifindex. Using ifindex will eventually save 32 bits on the
64
On Fri, 2006-30-06 at 19:33 +0200, Patrick McHardy wrote:
jamal wrote:
You are attempting to change architecture (which works just fine) in the
way you
think it should work - and then point to something as a bug because it
doesnt work the way you think it should work.
This is a
* jamal [EMAIL PROTECTED] 2006-06-30 15:34
I thought we went past that point already - and i made it clear that
the reference is _not_ taken in netif_receive_skb().
So assuming it is taken in mirred (i havent thought of where it is
decremented), why would using the ifindex be better?
The
From: jamal [EMAIL PROTECTED]
Date: Fri, 30 Jun 2006 15:40:26 -0400
My arguement was:
dev get by index per device will involve a a) lookup + b) incrementing
the refcount.
if i use the dev pointer in that path then it becomes only #b
in both cases, you need to increment the counter and
* jamal [EMAIL PROTECTED] 2006-06-30 15:59
One thing is clear in my mind at least (and i have said it several
times): I am not as good at the semantics as either yourself or Thomas
or Dave or Acme etc but i have tons of other things that compensate for.
Probably not as good is not the best
From: Thomas Graf [EMAIL PROTECTED]
Date: Fri, 30 Jun 2006 22:30:34 +0200
Lack of stylistic commas is less of a concern, rather the lack of
comments and notes where absolutely essential.
Yes, intent is very important to document when it is not
obvious.
Everyone is guilty of not doing this
On Fri, 2006-30-06 at 22:08 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-30 15:34
So assuming it is taken in mirred (i havent thought of where it is
decremented), why would using the ifindex be better?
The issue exists regardless of mirred/ifb. As soon as the packet is
On Fri, 2006-30-06 at 13:33 -0700, David Miller wrote:
From: Thomas Graf [EMAIL PROTECTED]
Date: Fri, 30 Jun 2006 22:30:34 +0200
Lack of stylistic commas is less of a concern, rather the lack of
comments and notes where absolutely essential.
Yes, intent is very important to document
* jamal [EMAIL PROTECTED] 2006-06-30 16:54
I agree - and i try hard to document but at times there's too much
and a line needs to be drawn.
As an example:
the eth-ifb-ifb case though is a very corner case. All the IMQ types
need to only redirect to one ifb; while i test it ive always seen the
Ok, I think found the last patch you posted, comments below (I have to
run off soon):
On Fri, 2006-30-06 at 19:13 +0200, Thomas Graf wrote:
There you go, leaves ifb broken as-is, at least prevents it
from crashing randomly when the input_dev disappears.
I hope the above comment does show up
* jamal [EMAIL PROTECTED] 2006-06-30 17:09
On Fri, 2006-30-06 at 19:13 +0200, Thomas Graf wrote:
Index: net-2.6.git/drivers/net/ifb.c
===
--- net-2.6.git.orig/drivers/net/ifb.c
+++ net-2.6.git/drivers/net/ifb.c
@@ -158,19
On Fri, 2006-30-06 at 23:10 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-30 16:54
I agree - and i try hard to document but at times there's too much
and a line needs to be drawn.
As an example:
the eth-ifb-ifb case though is a very corner case. All the IMQ types
need to
This is boring, I reversed everything to not change any semantics and
you still complain.
* jamal [EMAIL PROTECTED] 2006-06-30 17:35
On Fri, 2006-30-06 at 23:20 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-30 17:09
the ref inside the else below after changing input_dev
* jamal [EMAIL PROTECTED] 2006-06-30 17:31
Better to explain the reason for ifb first:
ifb exists initially as a replacement for IMQ.
1) qdiscs/policies that are per device as opposed to system wide.
This now allows for sharing.
2) Allows for queueing incoming traffic for shaping instead
On Sat, 2006-01-07 at 01:22 +0200, Thomas Graf wrote:
This is boring, I reversed everything to not change any semantics and
you still complain.
You reversed a bug that you had introduced. Do you want me to review
this patch or not now? Be a little reasonable please.
* jamal [EMAIL
jamal wrote:
On Fri, 2006-30-06 at 19:33 +0200, Patrick McHardy wrote:
Well, I thought I stay out of this, but since you mention me ..
I think you will add value to the discussion ;-
Regardless, we need to settle these kind of issues so we can work better
together. A while back i said was
On Sat, 2006-01-07 at 01:45 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-30 17:31
Better to explain the reason for ifb first:
ifb exists initially as a replacement for IMQ.
1) qdiscs/policies that are per device as opposed to system wide.
This now allows for sharing.
* jamal [EMAIL PROTECTED] 2006-06-28 09:46
Why not use iflink?
It exists, by definition, specifically as the ifindex of the down muxed
netdevice (should probably add that definition in the .h).
This is semantically different from a message to the stack which says
this came to you from input
I noticed the other email carrying a patch. Let me respond to this email
and hopefully that will address the patch.
On Thu, 2006-29-06 at 10:51 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-28 09:46
[..]
I disagree, iflink information is bogus once we start redirecting.
After
* jamal [EMAIL PROTECTED] 2006-06-29 19:23
What's currently
done in ifb:
skb-dev = skb-input_dev;
skb-input_dev = dev;
Confusing, isn't it?
not at all. Let me explain the design intent further below.
Then let me show you what your code does:
[mirred attached to
From: Thomas Graf [EMAIL PROTECTED]
Date: Fri, 30 Jun 2006 01:39:33 +0200
* jamal [EMAIL PROTECTED] 2006-06-29 19:23
I know your intent is noble in trying to save the 32 bits on 64 bit
machines (at least thats where your patch seems to have started) but the
cost:benefit ratio as i have
On Fri, 2006-30-06 at 01:39 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-29 19:23
not at all. Let me explain the design intent further below.
Then let me show you what your code does:
[mirred attached to filter on eth0 redirecting to ifb0]
tcf_mirred():
From: jamal [EMAIL PROTECTED]
Date: Thu, 29 Jun 2006 20:08:19 -0400
What am i missing?
on 64bit machine, does it not save 32 bits to use an ifindex as opposed
to the pointer?
The objects around it are pointers, which are 64-bit, and thus
the 32-bit object gets padded out to 64-bits in the
On Thu, 2006-29-06 at 17:12 -0700, David Miller wrote:
From: jamal [EMAIL PROTECTED]
Date: Thu, 29 Jun 2006 20:08:19 -0400
What am i missing?
on 64bit machine, does it not save 32 bits to use an ifindex as opposed
to the pointer?
The objects around it are pointers, which are 64-bit,
David Miller wrote:
I'm saying that, we don't need the refcount, just setting
the skb-input_index thing, unless someone actually cares
about the input device.
As long as the packet hits not paths that care about the
SKB input device, no atomic refcounts are taken. It's
just an integer sitting
* jamal [EMAIL PROTECTED] 2006-06-29 20:03
On Fri, 2006-30-06 at 01:39 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-29 19:23
not at all. Let me explain the design intent further below.
Then let me show you what your code does:
[mirred attached to filter on eth0
On Thu, 2006-29-06 at 17:29 -0700, David Miller wrote:
From: jamal [EMAIL PROTECTED]
Date: Thu, 29 Jun 2006 20:26:20 -0400
[..]
I see; i take it if things were moved around that may change?
Yes.
Ok, relief - so i was not totally unreasonable then ;-
Can you avoid doing the refcount?
* jamal [EMAIL PROTECTED] 2006-06-29 20:48
the ifb references it; only mirred redirects to the ifb at the moment.
You would need to increment in mirred, no?
Why do i feel i am missing something? ;-
The point is to avoid having an atomic operation for every packet
when setting iif in
On Fri, 2006-30-06 at 02:46 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-29 20:03
On Fri, 2006-30-06 at 01:39 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-29 19:23
not at all. Let me explain the design intent further below.
Then let me show you
On Fri, 2006-30-06 at 02:55 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-29 20:48
The point is to avoid having an atomic operation for every packet
when setting iif in netif_receive_skb(). If it was only for
mirred nobody would complain I guess.
I never intended to punish
* jamal [EMAIL PROTECTED] 2006-06-27 09:07
I am reading the thread backwards, so i may miss some of the obvious.
I also dont remember the exact discussion we had - but the consensus was
to leave the field setting as is.
Note the meta-setter (been sitting on it for too long) also set the
On Wed, 2006-28-06 at 12:18 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-27 09:07
[..]
Note the meta-setter (been sitting on it for too long) also set the
input_dev.
Could you share that piece of code so I don't have to duplicate
that effort?
I will look it up and send
* jamal [EMAIL PROTECTED] 2006-06-28 08:22
Let me see if i understood correctly:
you have a device from both vlan1 and vlan2 both being redirected to
ifb1? and vlan 3, 4= ifb2?
And i take it to make it interesting vlan1,2 on eth0 and vlan3,4 ontop
of eth1? note the iflink would/should reflect
On Wed, 2006-28-06 at 15:01 +0200, Thomas Graf wrote:
* jamal [EMAIL PROTECTED] 2006-06-28 08:22
[..]
Note, in such a case: iflink rewriting will do it just fine
but then you loose the original info (I think it would be good to not
loose such info to know the origin). I dont know if cmsg
On Tue, 2006-27-06 at 12:03 +0200, Thomas Graf wrote:
* Patrick McHardy [EMAIL PROTECTED] 2006-06-27 00:29
Doesn't sound too silly (actually quite nifty in my opinion),
but I'm not sure I understand correctly, binding to ifb doesn't
work since it changes both skb-dev and skb-input_dev.
I
Updating iif to the VLAN device helps keeping routing
namespaces defined in case packets from multiple VLANs
collapse on a single device again.
Signed-off-by: Thomas Graf [EMAIL PROTECTED]
Index: net-2.6.git/net/8021q/vlan_dev.c
===
Thomas Graf wrote:
Updating iif to the VLAN device helps keeping routing
namespaces defined in case packets from multiple VLANs
collapse on a single device again.
Signed-off-by: Thomas Graf [EMAIL PROTECTED]
Index: net-2.6.git/net/8021q/vlan_dev.c
From: Patrick McHardy [EMAIL PROTECTED]
Date: Mon, 26 Jun 2006 19:04:15 +0200
I know this was discussed before, but I can't remember the
exact outcome. Why don't we just unconditionally update iif
in netif_receive_skb()?
Software devices might have interesting semantics that would
make not
David Miller wrote:
From: Patrick McHardy [EMAIL PROTECTED]
Date: Mon, 26 Jun 2006 19:04:15 +0200
I know this was discussed before, but I can't remember the
exact outcome. Why don't we just unconditionally update iif
in netif_receive_skb()?
Software devices might have interesting
* David Miller [EMAIL PROTECTED] 2006-06-26 10:46
From: Patrick McHardy [EMAIL PROTECTED]
Date: Mon, 26 Jun 2006 19:04:15 +0200
I know this was discussed before, but I can't remember the
exact outcome. Why don't we just unconditionally update iif
in netif_receive_skb()?
Software
Thomas Graf wrote:
* David Miller [EMAIL PROTECTED] 2006-06-26 10:46
From: Patrick McHardy [EMAIL PROTECTED]
Date: Mon, 26 Jun 2006 19:04:15 +0200
I know this was discussed before, but I can't remember the
exact outcome. Why don't we just unconditionally update iif
in netif_receive_skb()?
Patrick McHardy wrote:
All my testing (quite a lot) in this area so far suggested that queueing
at ingress doesn't work and is actually counter-productive. It gets
worse with increasing signal propagation delays (signal in this case
means congestion indications). I actually wish I never
69 matches
Mail list logo
