patch: Action repeat

patch: Action repeat

[jamal]

[-- Attachment #1: Type: text/plain, Size: 123 bytes --]


Long standing bug.
Policy to repeat an action never worked.

signed-off-by: J Hadi Salim <hadi@cyberus.ca>

cheers,
jamal

[-- Attachment #2: repeat_p --]
[-- Type: text/plain, Size: 423 bytes --]

--- a/net/sched/act_api.c	2005/04/30 16:17:16	1.1
+++ b/net/sched/act_api.c	2005/04/30 16:17:44
@@ -171,10 +171,10 @@
 				skb->tc_verd = SET_TC_OK2MUNGE(skb->tc_verd);
 				skb->tc_verd = CLR_TC_MUNGED(skb->tc_verd);
 			}
-			if (ret != TC_ACT_PIPE)
-				goto exec_done;
 			if (ret == TC_ACT_REPEAT)
 				goto repeat;	/* we need a ttl - JHS */
+			if (ret != TC_ACT_PIPE)
+				goto exec_done;
 		}
 		act = a->next;
 	}

Re: patch: Action repeat

[Patrick McHardy]

jamal wrote:
> Long standing bug.
> Policy to repeat an action never worked.

I have a question regarding tcf_action_init() and skb->tc_classid.
tcf_action_init() overrides the classification result with
skb->tc_classid if set, but it isn't set anywhere. Do you have
future plans for it or can we remove it?

Regards
Patrick

Re: patch: Action repeat

[Patrick McHardy]

Patrick McHardy wrote:
> I have a question regarding tcf_action_init() and skb->tc_classid.
> tcf_action_init() overrides the classification result with

That should be tcf_action_exec(), sorry.

Re: patch: Action repeat

[jamal]

On Sat, 2005-30-04 at 19:08 +0200, Patrick McHardy wrote:
> Patrick McHardy wrote:
> > I have a question regarding tcf_action_init() and skb->tc_classid.
> > tcf_action_init() overrides the classification result with
> 
> That should be tcf_action_exec(), sorry.
> 

Actions can set it. 
Theres one in particular that i havent submitted yet that sets it
amongst other items (depending on policy). We discussed this a while
back when talking about ematches etc.

Unfortunately, classid is such a "hardcoded" concept in the qdiscs.

cheers,
jamal

Re: patch: Action repeat

[Patrick McHardy]

jamal wrote:
> Actions can set it. 
> Theres one in particular that i havent submitted yet that sets it
> amongst other items (depending on policy). We discussed this a while
> back when talking about ematches etc.

Since it only has such a short lifetime (action function sets it,
tcf_action_exec() clears it after changing classification result),
it would be less wasteful to just pass the classification result
to the actions. This would also avoid that skbs with tc_classid
already set can reach tcf_action_exec() (for example through mirred).

What do you think?

Regards
Patrick

Re: patch: Action repeat

[jamal]

On Sat, 2005-30-04 at 20:13 +0200, Patrick McHardy wrote:

> Since it only has such a short lifetime (action function sets it,
> tcf_action_exec() clears it after changing classification result),
> it would be less wasteful to just pass the classification result
> to the actions. This would also avoid that skbs with tc_classid
> already set can reach tcf_action_exec() (for example through mirred).
> 
> What do you think?

You mean not passing it back via skbs but through something else?
What do you have in mind?
It does sound distasteful for either changing the ->act()
parametrization just so we can have a classid passed back or provide a
spot for it in struct tc_action since only some actions will need to
change it. 


I see the issue with classid leaking - perhaps specific actions could
reset it when they steal packets? We should also reset it if the packet
is stolen.

cheers,
jamal

[In the long run really the classid setting should be per action
setting;
i.e when a user sets "flowid X:Y" they are explictly saying " action
metaset class X:Y";]

Re: patch: Action repeat

[Thomas Graf]

* jamal <1114890709.8929.147.camel@localhost.localdomain> 2005-04-30 15:51
> On Sat, 2005-30-04 at 20:13 +0200, Patrick McHardy wrote:
> 
> > Since it only has such a short lifetime (action function sets it,
> > tcf_action_exec() clears it after changing classification result),
> > it would be less wasteful to just pass the classification result
> > to the actions. This would also avoid that skbs with tc_classid
> > already set can reach tcf_action_exec() (for example through mirred).
> > 
> > What do you think?
> 
> You mean not passing it back via skbs but through something else?
> What do you have in mind?
> It does sound distasteful for either changing the ->act()
> parametrization just so we can have a classid passed back or provide a
> spot for it in struct tc_action since only some actions will need to
> change it. 

I've been using tc_classid to communicate between ingress and egress
without the need for netfilter but this is something personal. This
meant to remove the tc_classid = 0 in tcf_action_exec and a have
smallish action set it at ingress to pick it up again with the meta
ematch at egress.

> I see the issue with classid leaking - perhaps specific actions could
> reset it when they steal packets? We should also reset it if the packet
> is stolen.

Definitely.

I'm not yet certain on this subject, I have a strong feeling that
something like tc_classid will be needed but not as in its current
use. Can we postpone this for 1-2 weeks so I can submit my new
ematch patches? This would give us something to use as a basis for
a discussion.

Re: patch: Action repeat

[jamal]

On Sat, 2005-30-04 at 22:08 +0200, Thomas Graf wrote:

> I've been using tc_classid to communicate between ingress and egress
> without the need for netfilter but this is something personal. This
> meant to remove the tc_classid = 0 in tcf_action_exec and a have
> smallish action set it at ingress to pick it up again with the meta
> ematch at egress.
> 

I think we may have to define what the scope of classid is. It seems to
me the scope needs to be _local_ to either ingress or egress. 
OTOH, something like a fwmark is _global_. 
At least this is what my thoughts were when doing that piece.
Using those rules, the situation Patrick describes on violates this
(because stolen packets still maintain the classid), yours doesnt -
unless we change the scope of classid. 


> > I see the issue with classid leaking - perhaps specific actions could
> > reset it when they steal packets? We should also reset it if the packet
> > is stolen.
> 
> Definitely.

Just thinking about that: _exec() can reset classid if packet is stolen
and not transfer it back to classifier.
I think the forward path is to have the actions reset it. We would just
have to make it the rule described somewhere or have a macro someone
call every time they steal a packet... 

> I'm not yet certain on this subject, I have a strong feeling that
> something like tc_classid will be needed but not as in its current
> use. Can we postpone this for 1-2 weeks so I can submit my new
> ematch patches? This would give us something to use as a basis for
> a discussion.

If we are going to redefine the scope of where a classid applies, then
we can discuss it any time.

cheers,
jamal

Re: patch: Action repeat

[Thomas Graf]

* jamal <1114894202.8929.165.camel@localhost.localdomain> 2005-04-30 16:50
> I think we may have to define what the scope of classid is. It seems to
> me the scope needs to be _local_ to either ingress or egress. 
> OTOH, something like a fwmark is _global_. 
> At least this is what my thoughts were when doing that piece.
> Using those rules, the situation Patrick describes on violates this
> (because stolen packets still maintain the classid), yours doesnt -
> unless we change the scope of classid. 

Right, although I would define local as per device on either ingress
or egress. What I'm looking for in particular is a way to transfer
classification decisions from one device to another. A common case
for this is if a packet enters the packet, gets encapsulated and
leaves the host in some kind of tunnel. It's pretty hard to do
classification on the tunnel device so you want to do it before
the encapsulation and copy the result over to the tunnel. We can
use nfmark for this but i'd rather have a well defined variable
for this.

Re: patch: Action repeat

[jamal]

On Sat, 2005-30-04 at 23:55 +0200, Thomas Graf wrote:
> 
> Right, although I would define local as per device on either ingress
> or egress. What I'm looking for in particular is a way to transfer
> classification decisions from one device to another. A common case
> for this is if a packet enters the packet, gets encapsulated and
> leaves the host in some kind of tunnel. It's pretty hard to do
> classification on the tunnel device so you want to do it before
> the encapsulation and copy the result over to the tunnel. We can
> use nfmark for this but i'd rather have a well defined variable
> for this.

Perhaps we can reuse classid by flagging somewhere? 
A good place to do it is tc_verdict. There a few bits still left.
We could set a bit to say the meaning of classid to be global vs local.

cheers,
jamal

Re: patch: Action repeat

[Thomas Graf]

* jamal <1114900485.8929.171.camel@localhost.localdomain> 2005-04-30 18:34
> On Sat, 2005-30-04 at 23:55 +0200, Thomas Graf wrote:
> > 
> > Right, although I would define local as per device on either ingress
> > or egress. What I'm looking for in particular is a way to transfer
> > classification decisions from one device to another. A common case
> > for this is if a packet enters the packet, gets encapsulated and
> > leaves the host in some kind of tunnel. It's pretty hard to do
> > classification on the tunnel device so you want to do it before
> > the encapsulation and copy the result over to the tunnel. We can
> > use nfmark for this but i'd rather have a well defined variable
> > for this.
> 
> Perhaps we can reuse classid by flagging somewhere? 
> A good place to do it is tc_verdict. There a few bits still left.
> We could set a bit to say the meaning of classid to be global vs local.

Sounds good to me, I'm not quite sure if I still have a good enough
picture of your action code. Let's assume we have ematches and an
action setting the classid at ingress. The action sets the above flag
to state the global scope. The packet passes the stack and the
classid gets copied into the new encapsulated packet. On the egress
device we have something like a nop action assigning the classid
to res->classid. How do we make sure that the classid remains
untouched even if the packet passes a dummy device in between having
actions configured? Can we we still have functional actions on
this dummy device?

Re: patch: Action repeat

[Patrick McHardy]

jamal wrote:
> You mean not passing it back via skbs but through something else?
> What do you have in mind?
> It does sound distasteful for either changing the ->act()
> parametrization just so we can have a classid passed back or provide a
> spot for it in struct tc_action since only some actions will need to
> change it. 

I meant changing ->act() to have the same prototype as tcf_act_exec()
itself:

-       int     (*act)(struct sk_buff **, struct tc_action *);
+       int     (*act)(struct sk_buff **, struct tc_action *, struct 
tcf_result *);

> I see the issue with classid leaking - perhaps specific actions could
> reset it when they steal packets? We should also reset it if the packet
> is stolen.

Since its already reset after actions are executed, we only need to
additionally reset it for packets that take a different path.
At the moment I think this only happens with mirred.

Regards
Patrick

Re: patch: Action repeat

[Patrick McHardy]

Thomas Graf wrote:
> I'm not yet certain on this subject, I have a strong feeling that
> something like tc_classid will be needed but not as in its current
> use. Can we postpone this for 1-2 weeks so I can submit my new
> ematch patches? This would give us something to use as a basis for
> a discussion.

Sure, let's continue the discussion when we actually have users
of the field.

Regards
Patrick

Re: patch: Action repeat

[jamal]

On Sun, 2005-01-05 at 01:58 +0200, Thomas Graf wrote:
> * jamal <1114900485.8929.171.camel@localhost.localdomain> 2005-04-30 18:34
[..] 
> > Perhaps we can reuse classid by flagging somewhere? 
> > A good place to do it is tc_verdict. There a few bits still left.
> > We could set a bit to say the meaning of classid to be global vs local.
> 
> Sounds good to me, I'm not quite sure if I still have a good enough
> picture of your action code. Let's assume we have ematches and an
> action setting the classid at ingress. The action sets the above flag
> to state the global scope.

Which means the classid is not reset by exec().

>  The packet passes the stack and the
> classid gets copied into the new encapsulated packet. On the egress
> device we have something like a nop action assigning the classid
> to res->classid. How do we make sure that the classid remains
> untouched even if the packet passes a dummy device in between having
> actions configured? Can we we still have functional actions on
> this dummy device?

I would say that if dummy changes it because of a policy, then thats a
fair deal. i.e

filter blah blah \
action add meta classid global :23 

I am beginning to think that perhaps classid should stay as a local
scope metadata and what Patrick suggested maybe the way out. Although i
have to admit I dont like a generic function to have a parameter that
only a very small set of users find useful. If we are going to allow a
structure to be passed back and forth, perhaps it should also carry
other things (in addition to _result). Need to think a little.

cheers,
jamal

Re: patch: Action repeat

[Thomas Graf]

* jamal <1115035838.8929.236.camel@localhost.localdomain> 2005-05-02 08:10
> I would say that if dummy changes it because of a policy, then thats a
> fair deal. i.e
> 
> filter blah blah \
> action add meta classid global :23 

Absolutely, given it is requested by the user. My main concern are
dependencies on classid invisble to the user.

> I am beginning to think that perhaps classid should stay as a local
> scope metadata and what Patrick suggested maybe the way out. Although i
> have to admit I dont like a generic function to have a parameter that
> only a very small set of users find useful. If we are going to allow a
> structure to be passed back and forth, perhaps it should also carry
> other things (in addition to _result). Need to think a little.

What about if we introduce something like struct tcf_pkt_info as we
have it for ematches? I'm using it intensly to share information
from the classifier to ematches to extend and customize existing
classifier. We could declare tc_classid as being global by definition
and hide the current use in the API? I'd really like to be able to
transfer classification results from one device to another.

Re: patch: Action repeat

[David S. Miller]

On Sat, 30 Apr 2005 12:50:17 -0400
jamal <hadi@cyberus.ca> wrote:

> Long standing bug.
> Policy to repeat an action never worked.
> 
> signed-off-by: J Hadi Salim <hadi@cyberus.ca>

Applied, thanks Jamal.

Re: patch: Action repeat

[jamal]

On Mon, 2005-02-05 at 17:06 +0200, Thomas Graf wrote:
> * jamal <1115035838.8929.236.camel@localhost.localdomain> 2005-05-02 08:10
[..]
> > I am beginning to think that perhaps classid should stay as a local
> > scope metadata and what Patrick suggested maybe the way out. Although i
> > have to admit I dont like a generic function to have a parameter that
> > only a very small set of users find useful. If we are going to allow a
> > structure to be passed back and forth, perhaps it should also carry
> > other things (in addition to _result). Need to think a little.
> 
> What about if we introduce something like struct tcf_pkt_info as we
> have it for ematches? I'm using it intensly to share information
> from the classifier to ematches to extend and customize existing
> classifier. 

Basically, something along those lines (eg struct tca_pkt_info) in which
the tcf_result is one of the components should do it. 

I would be satisfied with this being the structure in the ->act()
parameters because then it could also be used to pass action-metadata
around (no action written so far needs such coordination, but its been
one of those things i have been thinking of for some dynamic creations
for example  where the return code is insufficient to describe things).
Patrick, either you or i could do it. It doesnt matter if at the moment
the structure only contains tcf_result or elements of tcf_result because
i will add more to it later. Then we could kill access to tc_classid in
exec()

> We could declare tc_classid as being global by definition
> and hide the current use in the API? I'd really like to be able to
> transfer classification results from one device to another.

since tc_classid suddenly becomes available theres no question about the
need for it being global - which is selectable at the meta action.

Global I believe means you dont reset it when you clone/copy.
skb->tc_verd is only cleared when we free the skb at the moment and
transfered when we clone or copy. A bit or two could be reserved in the
tc_verd to say "clear tc_classid" and have the meta action decide if it
is global(dont clear) or not(clear - current behavior) during
clone/copy . Does this sound reasonable?
 
cheers,
jamal

Re: patch: Action repeat

[Patrick McHardy]

jamal wrote:
> Patrick, either you or i could do it. It doesnt matter if at the moment
> the structure only contains tcf_result or elements of tcf_result because
> i will add more to it later. Then we could kill access to tc_classid in
> exec()

I'll do it, I have a few patches touching this code lying around anyway

Regards
Patrick

Re: patch: Action repeat

[Thomas Graf]

* jamal <1115207194.7665.109.camel@localhost.localdomain> 2005-05-04 07:46
> Basically, something along those lines (eg struct tca_pkt_info) in which
> the tcf_result is one of the components should do it. 
> 
> I would be satisfied with this being the structure in the ->act()
> parameters because then it could also be used to pass action-metadata
> around (no action written so far needs such coordination, but its been
> one of those things i have been thinking of for some dynamic creations
> for example  where the return code is insufficient to describe things).
> Patrick, either you or i could do it. It doesnt matter if at the moment
> the structure only contains tcf_result or elements of tcf_result because
> i will add more to it later. Then we could kill access to tc_classid in
> exec()

Sounds good.

> Global I believe means you dont reset it when you clone/copy.
> skb->tc_verd is only cleared when we free the skb at the moment and
> transfered when we clone or copy. A bit or two could be reserved in the
> tc_verd to say "clear tc_classid" and have the meta action decide if it
> is global(dont clear) or not(clear - current behavior) during
> clone/copy . Does this sound reasonable?

I have no objections but fail to see why we want to clear it anyway?

Re: patch: Action repeat

[jamal]

On Wed, 2005-04-05 at 14:31 +0200, Thomas Graf wrote:
> * jamal <1115207194.7665.109.camel@localhost.localdomain> 2005-05-04 07:46

> > tc_verd to say "clear tc_classid" and have the meta action decide if it
> > is global(dont clear) or not(clear - current behavior) during
> > clone/copy . Does this sound reasonable?
> 
> I have no objections but fail to see why we want to clear it anyway?
> 

If its scope is local i.e for one device, then not reseting could
confuse the next device that sees it and tries to classify on it.

cheers,
jamal

Re: patch: Action repeat

[Thomas Graf]

* jamal <1115211549.7665.140.camel@localhost.localdomain> 2005-05-04 08:59
> On Wed, 2005-04-05 at 14:31 +0200, Thomas Graf wrote:
> > * jamal <1115207194.7665.109.camel@localhost.localdomain> 2005-05-04 07:46
> 
> > > tc_verd to say "clear tc_classid" and have the meta action decide if it
> > > is global(dont clear) or not(clear - current behavior) during
> > > clone/copy . Does this sound reasonable?
> > 
> > I have no objections but fail to see why we want to clear it anyway?
> > 
> 
> If its scope is local i.e for one device, then not reseting could
> confuse the next device that sees it and tries to classify on it.

OK, so we're not talking about a reset in action_exec() but rather
in tc_classify() or enqueue()?

Re: patch: Action repeat

[Thomas Graf]

* Thomas Graf <20050504132822.GB18452@postel.suug.ch> 2005-05-04 15:28
> * jamal <1115211549.7665.140.camel@localhost.localdomain> 2005-05-04 08:59
> > On Wed, 2005-04-05 at 14:31 +0200, Thomas Graf wrote:
> > > * jamal <1115207194.7665.109.camel@localhost.localdomain> 2005-05-04 07:46
> > 
> > > > tc_verd to say "clear tc_classid" and have the meta action decide if it
> > > > is global(dont clear) or not(clear - current behavior) during
> > > > clone/copy . Does this sound reasonable?
> > > 
> > > I have no objections but fail to see why we want to clear it anyway?
> > > 
> > 
> > If its scope is local i.e for one device, then not reseting could
> > confuse the next device that sees it and tries to classify on it.
> 
> OK, so we're not talking about a reset in action_exec() but rather
> in tc_classify() or enqueue()?

Sorry, little bit to vague, what I mean is before the first call to
tc_classify() or enqueue() on a new device.

Re: patch: Action repeat

[jamal]

On Wed, 2005-04-05 at 15:28 +0200, Thomas Graf wrote:
> * jamal <1115211549.7665.140.camel@localhost.localdomain> 2005-05-04 08:59
> > On Wed, 2005-04-05 at 14:31 +0200, Thomas Graf wrote:
> > > * jamal <1115207194.7665.109.camel@localhost.localdomain> 2005-05-04 07:46
> > 
> > > > tc_verd to say "clear tc_classid" and have the meta action decide if it
> > > > is global(dont clear) or not(clear - current behavior) during
> > > > clone/copy . Does this sound reasonable?
> > > 
> > > I have no objections but fail to see why we want to clear it anyway?
> > > 
> > 
> > If its scope is local i.e for one device, then not reseting could
> > confuse the next device that sees it and tries to classify on it.
> 
> OK, so we're not talking about a reset in action_exec() but rather
> in tc_classify() or enqueue()?

in skb_clone() and friends.
Look at CONFIG_NET_CLS_ACT in net/core/skbuff.c

cheers,
jamal

Re: patch: Action repeat

[Thomas Graf]

* jamal <1115213600.7665.166.camel@localhost.localdomain> 2005-05-04 09:33
> On Wed, 2005-04-05 at 15:28 +0200, Thomas Graf wrote:
> > * jamal <1115211549.7665.140.camel@localhost.localdomain> 2005-05-04 08:59
> > > If its scope is local i.e for one device, then not reseting could
> > > confuse the next device that sees it and tries to classify on it.
> > 
> > OK, so we're not talking about a reset in action_exec() but rather
> > in tc_classify() or enqueue()?
> 
> in skb_clone() and friends.
> Look at CONFIG_NET_CLS_ACT in net/core/skbuff.c

Yes this solves the case for dummy devices etc but how would
this cause a reset on the way from ingress to egress?

Re: patch: Action repeat

[jamal]

On Wed, 2005-04-05 at 15:48 +0200, Thomas Graf wrote:
> * jamal <1115213600.7665.166.camel@localhost.localdomain> 2005-05-04 09:33

> > in skb_clone() and friends.
> > Look at CONFIG_NET_CLS_ACT in net/core/skbuff.c
> 
> Yes this solves the case for dummy devices etc but how would
> this cause a reset on the way from ingress to egress?

If the verdict is not to reset, there should be no clearing of those
fields from ingress -> egress until the skb is either freed or someone
else along the path resets it. Cloning or copying inherits. Am i missing
something?

cheers,
jamal 

Re: patch: Action repeat

[Thomas Graf]

* jamal <1115214782.7665.184.camel@localhost.localdomain> 2005-05-04 09:53
> On Wed, 2005-04-05 at 15:48 +0200, Thomas Graf wrote:
> > * jamal <1115213600.7665.166.camel@localhost.localdomain> 2005-05-04 09:33
> 
> > > in skb_clone() and friends.
> > > Look at CONFIG_NET_CLS_ACT in net/core/skbuff.c
> > 
> > Yes this solves the case for dummy devices etc but how would
> > this cause a reset on the way from ingress to egress?
> 
> If the verdict is not to reset, there should be no clearing of those
> fields from ingress -> egress until the skb is either freed or someone
> else along the path resets it. Cloning or copying inherits. Am i missing
> something?

I guess not but we might have a different understanding of when to
reset. From my point of view the only reason to reset any meta data
is to provide a certain scope a set of new fresh and clean sheets
to play around. Assuming we define global as everything and local
as per device/(ingress|egress) then we definitely need to invoke
a reset on the way over to egress. 

Re: patch: Action repeat

[jamal]

On Wed, 2005-04-05 at 16:05 +0200, Thomas Graf wrote:


>  Assuming we define global as everything and local
> as per device/(ingress|egress) then we definitely need to invoke
> a reset on the way over to egress. 

Agreed. So shall we stick then to just make it a global-only?

cheers,
jamal

Re: patch: Action repeat

[Thomas Graf]

* jamal <1115216629.7906.4.camel@localhost.localdomain> 2005-05-04 10:23
> On Wed, 2005-04-05 at 16:05 +0200, Thomas Graf wrote:
> 
> 
> >  Assuming we define global as everything and local
> > as per device/(ingress|egress) then we definitely need to invoke
> > a reset on the way over to egress. 
> 
> Agreed. So shall we stick then to just make it a global-only?

As long as we can't find a way to have a well defined local
scope which is both easy to implement and easy to understand
for the user a global-only scope with the ability for the user
to reset the value via the meta action is most reasonable to me.
We need to define this well because it will get to be the backend
for any advanced classyfing over multiple devices. I will give it
some more thinking in the background, in the meantime I suggest
to implement it global-only for now. The global classid issue is
the next item on my list and I should be able to come up with
something this week after posting rtnetlink neighbour tables
patches and the new generic textsearch API + skb search bits +
textsearch ematch which will provide optimized textsearching
facitilies including a simple regular expression for both linear
and non-linear skbs.

Re: patch: Action repeat

[jamal]

On Wed, 2005-04-05 at 16:53 +0200, Thomas Graf wrote:
> * jamal <1115216629.7906.4.camel@localhost.localdomain> 2005-05-04 10:23

> > Agreed. So shall we stick then to just make it a global-only?
> 
> As long as we can't find a way to have a well defined local
> scope which is both easy to implement and easy to understand
> for the user a global-only scope with the ability for the user
> to reset the value via the meta action is most reasonable to me.

ok - i will wait to see the patches from Patrick out first then do this.
Its actually a simple change.

cheers,
jamal