Re: [PATCH 2.6.23+] ingress classify to [nf]mark

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[Patrick McHardy]

Dzianis Kahanovich wrote:
> --- linux-2.6.23-gentoo-r2/net/sched/sch_ingress.c
> +++ linux-2.6.23-gentoo-r2.fixed/net/sched/sch_ingress.c
> @@ -161,2 +161,5 @@
>              skb->tc_index = TC_H_MIN(res.classid);
> +#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
> +            skb->mark = 
> (skb->mark&(res.classid>>16))|TC_H_MIN(res.classid);
> +#endif
>          default:


Behaviour like this shouldn't depend on compile-time options.

[PATCH 2.6.23+] ingress classify to [nf]mark

[Dzianis Kahanovich]

To "classid x:y" = "mark=mark&x|y" ("classid :y" = "-j MARK --set-mark y", etc).

--- linux-2.6.23-gentoo-r2/net/sched/Kconfig
+++ linux-2.6.23-gentoo-r2.fixed/net/sched/Kconfig
@@ -222,6 +222,16 @@
  	  To compile this code as a module, choose M here: the
  	  module will be called sch_ingress.

+config NET_SCH_INGRESS_TC2MARK
+	bool "ingress classify -> mark"
+	depends on NET_SCH_INGRESS && NET_CLS_ACT
+	---help---
+	  This enables access to "mark" value via "classid"
+	  Example: set "tc filter ... flowid|classid 1:2"
+	  eq "netfilter mark" mark=mark&1|2
+	
+	  But classid may be undefined (?) - use "flowid :0".
+
  comment "Classification"

  config NET_CLS
--- linux-2.6.23-gentoo-r2/net/sched/sch_ingress.c
+++ linux-2.6.23-gentoo-r2.fixed/net/sched/sch_ingress.c
@@ -161,2 +161,5 @@
  			skb->tc_index = TC_H_MIN(res.classid);
+#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
+			skb->mark = (skb->mark&(res.classid>>16))|TC_H_MIN(res.classid);
+#endif
  		default:


-- 
WBR,
Denis Kaganovich,  mahatma@eu.by  http://mahatma.bspu.unibel.by

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[jamal]

On Thu, 2008-10-01 at 17:05 -0200, Dzianis Kahanovich wrote:
> To "classid x:y" = "mark=mark&x|y" ("classid :y" = "-j MARK --set-mark y", etc).
> 
> --- linux-2.6.23-gentoo-r2/net/sched/Kconfig
> +++ linux-2.6.23-gentoo-r2.fixed/net/sched/Kconfig
> @@ -222,6 +222,16 @@
[..]
>   			skb->tc_index = TC_H_MIN(res.classid);
> +#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
> +			skb->mark = (skb->mark&(res.classid>>16))|TC_H_MIN(res.classid);
> +#endif
>   		default:


Please either use ipt action and netfilter fwmarker for this activity or
create a new action. 
If you choose the later (example because you want to dynamically compute
the mark), look at net/sched/act_simple.c to start from and i can help
you if you have any questions.
 
If you want to use ipt action, the syntax would be something like:

---
tc qdisc add dev XXX ingress
tc filter add dev XXX parent ffff: protocol ip prio 5 \
u32 blah bleh \
flowid 1:12 action ipt -j mark --set-mark 13 
-----

cheers,
jamal

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[jamal]

On Fri, 2008-11-01 at 15:24 -0200, Dzianis Kahanovich wrote:
> jamal wrote:


> > tc qdisc add dev XXX ingress
> > tc filter add dev XXX parent ffff: protocol ip prio 5 \
> > u32 blah bleh \
> > flowid 1:12 action ipt -j mark --set-mark 13 
> 
> Yes, I do so. But there are simple:
> ---
> if [[ $[TC_INDEX2MARK] == 0 ]] ; then
>   c=${c//action ipt -j MARK --set-mark /flowid :}
> fi
> $c
> ---

I didnt quiet understand what you have above. Does your script above
read the flowid and sets the MARK to some dynamic value based on flowid?
if thats what you are doing - it sounds sensible and much more clever
than what is posted. And it doesnt require any kernel patch.

> Simpliest:
> --- linux-2.6.23-gentoo-r2/net/sched/sch_ingress.c
> +++ linux-2.6.23-gentoo-r2.fixed/net/sched/sch_ingress.c
> @@ -222,6 +222,16 @@
> -   			skb->tc_index = TC_H_MIN(res.classid);
> +   			skb->tc_index = TC_H_MIN(mark=res.classid);

Just write a metaset action and you can have all sorts of policies on
what tc_index, mark etc you want. It is something thats needed in any
case.
When we did tc_index it made sense then because it was for "tc" to use
some default policy. Enforcing policies in the kernel is not the best
thing to do; as an example you want to specify the polciy for mark to
be: classid major>>16|minor. I am sure you have good reasons; however,
for the next person who wants to set it it major>>8|minor for their own
good reason, theres conflict.  
My offer to help you is still open.

cheers,
jamal

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[Dzianis Kahanovich]

jamal wrote:

>> To "classid x:y" = "mark=mark&x|y" ("classid :y" = "-j MARK --set-mark y", etc).
>>
>> --- linux-2.6.23-gentoo-r2/net/sched/Kconfig
>> +++ linux-2.6.23-gentoo-r2.fixed/net/sched/Kconfig
>> @@ -222,6 +222,16 @@
> [..]
>>   			skb->tc_index = TC_H_MIN(res.classid);
>> +#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
>> +			skb->mark = (skb->mark&(res.classid>>16))|TC_H_MIN(res.classid);
>> +#endif
>>   		default:
> 
> 
> Please either use ipt action and netfilter fwmarker for this activity or

Sorry. There are only unsuccessful attempt to popularize my working solution.
Really I just use "#define tc_index mark" (in skbuff.h or sch_ingress.c) or 
something like this:

--- linux-2.6.23-gentoo-r2/net/sched/Kconfig
+++ linux-2.6.23-gentoo-r2.fixed/net/sched/Kconfig
@@ -222,6 +222,16 @@
  	  To compile this code as a module, choose M here: the
  	  module will be called sch_ingress.

+config NET_SCH_INGRESS_TC2MARK
+	bool "ingress tc_index -> mark"
+	depends on NET_SCH_INGRESS && NET_CLS_ACT
+	---help---
+	  This enables access to "mark" value via "tc_index" alias
+	  in ingress and unify this values (usage example: set "flowid :2"
+	  in ingress and use it value as "mark" in any way - netfilter, etc).
+	
+	  But tc_index may be undefined - use "flowid :0".
+
  comment "Classification"

  config NET_CLS
--- linux-2.6.23-gentoo-r2/net/sched/sch_ingress.c
+++ linux-2.6.23-gentoo-r2.fixed/net/sched/sch_ingress.c
@@ -18,6 +18,9 @@
  #include <net/netlink.h>
  #include <net/pkt_sched.h>

+#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
+#define tc_index mark
+#endif

  #undef DEBUG_INGRESS



> create a new action. 
> If you choose the later (example because you want to dynamically compute
> the mark), look at net/sched/act_simple.c to start from and i can help
> you if you have any questions.
>  
> If you want to use ipt action, the syntax would be something like:
> 
> ---
> tc qdisc add dev XXX ingress
> tc filter add dev XXX parent ffff: protocol ip prio 5 \
> u32 blah bleh \
> flowid 1:12 action ipt -j mark --set-mark 13 

Yes, I do so. But there are simple:
---
if [[ $[TC_INDEX2MARK] == 0 ]] ; then
  c=${c//action ipt -j MARK --set-mark /flowid :}
fi
$c
---

Simpliest:
--- linux-2.6.23-gentoo-r2/net/sched/sch_ingress.c
+++ linux-2.6.23-gentoo-r2.fixed/net/sched/sch_ingress.c
@@ -222,6 +222,16 @@
-   			skb->tc_index = TC_H_MIN(res.classid);
+   			skb->tc_index = TC_H_MIN(mark=res.classid);


-- 
WBR,
Denis Kaganovich,  mahatma@eu.by  http://mahatma.bspu.unibel.by

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[Dzianis Kahanovich]

Patrick McHardy wrote:

>> --- linux-2.6.23-gentoo-r2/net/sched/sch_ingress.c
>> +++ linux-2.6.23-gentoo-r2.fixed/net/sched/sch_ingress.c
>> @@ -161,2 +161,5 @@
>>              skb->tc_index = TC_H_MIN(res.classid);
>> +#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
>> +            skb->mark = 
>> (skb->mark&(res.classid>>16))|TC_H_MIN(res.classid);
>> +#endif
>>          default:
> 
> 
> Behaviour like this shouldn't depend on compile-time options.

Also I want to move it outside of NET_CLS_ACT dependence, but unsure in 
behaviour understanding without NET_CLS_ACT.

But there are reduse code.

-- 
WBR,
Denis Kaganovich,  mahatma@eu.by  http://mahatma.bspu.unibel.by

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[Dzianis Kahanovich]

jamal wrote:

>> Yes, I do so. But there are simple:
>> ---
>> if [[ $[TC_INDEX2MARK] == 0 ]] ; then
==1
>>   c=${c//action ipt -j MARK --set-mark /flowid :}
    c=${c//action ipt -j MARK --set-mark 0x/flowid :}
>> fi
>> $c
>> ---
> 
> I didnt quiet understand what you have above. Does your script above
> read the flowid and sets the MARK to some dynamic value based on flowid?
> if thats what you are doing - it sounds sensible and much more clever
> than what is posted. And it doesnt require any kernel patch.

I suggest just to use classid to toggle mark/nfmark in ingress. I see, classid
are near unused in ingress (no classes, etc) and for many solutions classid in
ingress filters may be used only for nfmarking. Also I suggest to use both
parts (major & minor) of classid - major may be "and" value, minor - "or". In
current place it may be useful only for (if, unsure) overriting netfilter
"raw" table marks, but if it will be moved outside current "CLS_ACT" block -
tc filter rules may operate mark bits more useful.

About script example:
While I compose filter, I check flag ($TC_INDEX2MARK), tells me are patch
applied or no. If no - I use usual "-j MARK --set-mark", else I use classid to
change mark. All in ingress only. For example:
tc filter add dev eth0 parent ffff: protocol ip u32 ... action ipt -j MARK 0x10
are cname to:
tc filter add dev eth0 parent ffff: protocol ip u32 ... flowid :10

- it use less code/modules and, in many cases, may be single/main goal to
ingress usage - pre-marking packets.

>> Simpliest:
>> --- linux-2.6.23-gentoo-r2/net/sched/sch_ingress.c
>> +++ linux-2.6.23-gentoo-r2.fixed/net/sched/sch_ingress.c
>> @@ -222,6 +222,16 @@
>> -   			skb->tc_index = TC_H_MIN(res.classid);
>> +   			skb->tc_index = TC_H_MIN(mark=res.classid);
> 
> Just write a metaset action and you can have all sorts of policies on
> what tc_index, mark etc you want. It is something thats needed in any
> case.
> When we did tc_index it made sense then because it was for "tc" to use
> some default policy. Enforcing policies in the kernel is not the best
> thing to do; as an example you want to specify the polciy for mark to
> be: classid major>>16|minor. I am sure you have good reasons; however,
> for the next person who wants to set it it major>>8|minor for their own
> good reason, theres conflict.  
> My offer to help you is still open.

OK, I understand there are not too transparent for future usage, but I see too
few applications for ingress/classid will conflicting with.

Thanx, I will try to understand "metaset actions", but I think it will be not
so elegant for my usage then my "#define tc_index mark" in the beginning of
sch_ingress.c. Or may be I will use "and/or" behaviour, but now "#define
tc_index mark" works on my router many month (I may use also -j MARK - with
one flag in my script, but there are lot of unuseful code).

This code (ingress/classifying[/CLS_ACT]) are executing everywhen and I
suggest changes from none (changing target variable from "tc_index" to "mark")
to few "and/or" atomic operations for useful functionality. With
"mark=res.classid" only (I may use self, but not suggest to kernel) it even
less code then default (no TC_H_MIN) and fully satisfy to many goals (traffic
marking without netfilter, but compatible with it).

-- 
WBR,
Denis Kaganovich,  mahatma@eu.by  http://mahatma.bspu.unibel.by

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[jamal]

On Fri, 2008-11-01 at 18:42 -0200, Dzianis Kahanovich wrote:

> About script example:
> While I compose filter, I check flag ($TC_INDEX2MARK), tells me are patch
> applied or no. If no - I use usual "-j MARK --set-mark", else I use classid to
> change mark. All in ingress only. For example:
> tc filter add dev eth0 parent ffff: protocol ip u32 ... action ipt -j MARK 0x10
> are cname to:
> tc filter add dev eth0 parent ffff: protocol ip u32 ... flowid :10

I thought you were doing something like this (to achieve your policy):

----------
major=1
minor=12
mark=`expr $major + $minor`
#
tc qdisc add dev XXX ingress
tc filter add dev XXX parent ffff: protocol ip prio 5 \
u32 blah bleh \
flowid $major:$minor action \
ipt -j mark --set-mark $mark
---------------

> - it use less code/modules and, in many cases, may be single/main goal to
> ingress usage - pre-marking packets.

That is true and you would also have one less line in your policy; as an
example in above the line "ipt -j mark --set-mark $mark" would be
unnecessary; however, all the other lines in the policy setting _will be
necessary_. And this + the fact there are many other values/shapes the
default policy could take is essentially whats bothering me. 

In any case, scanning the current code it seems mark is no longer
considered a netfilter-only metadatum - so it may not be semantically as
obscene as i felt earlier; Can you pick something simpler for policy?
example set the mark to whatever tc_index gets set?
If you still could write the metadata action, we could use it to
override mark, tc_index etc in addition.

cheers,
jamal

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[Dzianis Kahanovich]

I in doubts only about "action continue".
To "and/or" behaviour one of best usage are (example):

# set bit 2 of mark to 0 (mark&0xfd|0) and continue
tc filter add ... prio 1 ... flowid fd:0 action continue
# continue
tc filter add ... prio 2 ...

- in current ingress_enqueue() code IMHO "case TC_ACT_OK:" will not reached 
for action continue. I use old (mark=...) solution only by this.

I think, "skb->mark = (skb->mark&(res.classid>>16))|TC_H_MIN(res.classid);" 
must be in the end of ingress_enqueue() before "return result". And not 
depended to "NET_CLS_ACT". But while not test it.
Or this:
---
#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
#ifdef CONFIG_NET_CLS_ACT
	skb->mark = (skb->mark&(res.classid>>16))|TC_H_MIN(res.classid);
#else
	skb->mark = res.classid;
#endif
#endif
         return result;
}


jamal wrote:

>> While I compose filter, I check flag ($TC_INDEX2MARK), tells me are patch
>> applied or no. If no - I use usual "-j MARK --set-mark", else I use classid to
>> change mark. All in ingress only. For example:
>> tc filter add dev eth0 parent ffff: protocol ip u32 ... action ipt -j MARK 0x10
>> are cname to:
>> tc filter add dev eth0 parent ffff: protocol ip u32 ... flowid :10
> 
> I thought you were doing something like this (to achieve your policy):
> 
> ----------
> major=1
> minor=12
> mark=`expr $major + $minor`
> #
> tc qdisc add dev XXX ingress
> tc filter add dev XXX parent ffff: protocol ip prio 5 \
> u32 blah bleh \
> flowid $major:$minor action \
> ipt -j mark --set-mark $mark
> ---------------
> 
>> - it use less code/modules and, in many cases, may be single/main goal to
>> ingress usage - pre-marking packets.
> 
> That is true and you would also have one less line in your policy; as an
> example in above the line "ipt -j mark --set-mark $mark" would be
> unnecessary; however, all the other lines in the policy setting _will be
> necessary_. And this + the fact there are many other values/shapes the
> default policy could take is essentially whats bothering me. 
> 
> In any case, scanning the current code it seems mark is no longer
> considered a netfilter-only metadatum - so it may not be semantically as
> obscene as i felt earlier; Can you pick something simpler for policy?
> example set the mark to whatever tc_index gets set?
> If you still could write the metadata action, we could use it to
> override mark, tc_index etc in addition.
> 
> cheers,
> jamal
> 
> 
> 


-- 
WBR,
Denis Kaganovich,  mahatma@eu.by  http://mahatma.bspu.unibel.by

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[jamal]

Hi,


Please CC me in your responses (the way i do when i respond to you),
that way my filters prioritize your email. 

On Sat, 2008-12-01 at 15:56 -0200, Dzianis Kahanovich wrote:
> I in doubts only about "action continue".
> To "and/or" behaviour one of best usage are (example):

I dont think you should be touching the action part at all primarily
because actions can set the mark after classification. 
The action code (not the default) should be the override. IOW, if i
specify a ipt mark of some value i would expect that value to be what
goes into the network stack and not the default value you want. Same if
i had a series of actions which override each others settings of mark.

When we have a metadata action, we can remove the setting of tcindex
in the action OK result case (for now it doesnt harm).

In other words, just set the #ifndef action to set both the tcindex and
mark to some policy;

> # set bit 2 of mark to 0 (mark&0xfd|0) and continue
> tc filter add ... prio 1 ... flowid fd:0 action continue
> # continue
> tc filter add ... prio 2 ...
> 
> - in current ingress_enqueue() code IMHO "case TC_ACT_OK:" will not reached 
> for action continue. I use old (mark=...) solution only by this.
> 
> I think, "skb->mark = (skb->mark&(res.classid>>16))|TC_H_MIN(res.classid);" 
> must be in the end of ingress_enqueue() before "return result". And not 
> depended to "NET_CLS_ACT". But while not test it.
> Or this:
> ---
> #ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
> #ifdef CONFIG_NET_CLS_ACT
> 	skb->mark = (skb->mark&(res.classid>>16))|TC_H_MIN(res.classid);
> #else
> 	skb->mark = res.classid;
> #endif
> #endif

Please refer to what i said above; if what i said still doesnt make
sense i can create (the simple) patch.

cheers,
jamal

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[jamal]

On Mon, 2008-14-01 at 13:40 -0200, Dzianis Kahanovich wrote:
> jamal wrote:

> Yes, I only do it by inertia after "#define tc_index mark".

And i am afraid this bothers me greatly.
You already have ways to achieve what you need by setting proper policy,
the difference in configuration is an extra one policy line you have to
type in. Adding yet another #ifdef is really going overboard.

> I not understand why "tc_index" changed in this place. 1) there are ingress 2) 
> there are "OK" action. Are "tc_index" will not changed after: "tc filter add 
> dev eth0 parent ffff: ... flowid 1:1 action continue" ? In general - are 
> tc_index useful in ingress? (may be tc_index used in [nf]mark-style, but even 
> in netfilter it feature migrate - IMHO, may be I time to time do not see in 
> needed place)

tc_index could be used for classification actually. If you "continue"
you could hit another classifier which looks at it.

> Sorry, I just change focus from existing "tc_index=..." to common behaviour ;)

> [...]
> > Please refer to what i said above; if what i said still doesnt make
> > sense i can create (the simple) patch.
> 
> A bit vague... sorry...

I mean:

#ifdef CONFIG_NET_CLS_ACT
.... leave this part alone which already sets tc_index ...
#else
...set tc_index and mark here ...
#endif

And when we have a metadata action - we remove setting of tc_index from
#ifdef CONFIG_NET_CLS_ACT

Did that make sense?

cheers,
jamal

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[Dzianis Kahanovich]

jamal wrote:

>> I in doubts only about "action continue".
>> To "and/or" behaviour one of best usage are (example):
> 
> I dont think you should be touching the action part at all primarily
> because actions can set the mark after classification. 

Yes, I only do it by inertia after "#define tc_index mark".

I not understand why "tc_index" changed in this place. 1) there are ingress 2) 
there are "OK" action. Are "tc_index" will not changed after: "tc filter add 
dev eth0 parent ffff: ... flowid 1:1 action continue" ? In general - are 
tc_index useful in ingress? (may be tc_index used in [nf]mark-style, but even 
in netfilter it feature migrate - IMHO, may be I time to time do not see in 
needed place)

Sorry, I just change focus from existing "tc_index=..." to common behaviour ;)

[...]
> Please refer to what i said above; if what i said still doesnt make
> sense i can create (the simple) patch.

A bit vague... sorry...

-- 
WBR,
Denis Kaganovich,  mahatma@eu.by  http://mahatma.bspu.unibel.by

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[Dzianis Kahanovich]

jamal wrote:

May be I am mix in mind other code (multi-class loop/walking) and this code. I 
am deprogramming... ;)

>> Sorry, I just change focus from existing "tc_index=..." to common behaviour ;)
> 
>> [...]
>>> Please refer to what i said above; if what i said still doesnt make
>>> sense i can create (the simple) patch.
>> A bit vague... sorry...
> 
> I mean:
> 
> #ifdef CONFIG_NET_CLS_ACT
> .... leave this part alone which already sets tc_index ...
> #else
> ...set tc_index and mark here ...
> #endif
> 
> And when we have a metadata action - we remove setting of tc_index from
> #ifdef CONFIG_NET_CLS_ACT
> 
> Did that make sense?

After current "#endif" - may be.

What "result" are with:
1) no filters?
2) 1 filter only, with "action continue"?

-- 
WBR,
Denis Kaganovich,  mahatma@eu.by  http://mahatma.bspu.unibel.by

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[jamal]

On Mon, 2008-14-01 at 20:20 -0200, Dzianis Kahanovich wrote:
> jamal wrote:
[..] 

> > Did that make sense?
>
> After current "#endif" - may be.

I am afraid that would be counter to expected behavior. 
Default is meant to apply when no value has been defined. Mark of 0 for
example doesnt mean "default". Let me demonstrate with the ifdefs again
with some arbitrary example:

-----------------
#ifdef CONFIG_NET_CLS_ACT
..classify ...
    .. action 1 sets mark to 0x11111
    .. action 2 checks some state and conditionally let action 3 execute
    .. action 3 sets mark to 0

if OK is returned set tc_index based on classid

#else // no actions compiled
..classify
.... jamal suggests: set default mark and tc_index for ingress here
#endif

// mahatma wants to set default for mark and tcindex here 
// so it works for both actions and none-action code
------------------------

Lets look at the case of actions compiled in:
I have defined my policies (in user space) so that the mark can be set
to either 0 or 0x1111 depending on some runtime state. 
Your default (kernel) code is now going to overide my policy - which is
bad. Even in the case of OK being returned, it is wrong to set tc_index;
unfortunately, we dont have an action that can set tc_index today; if we
did, we would need to remove that setting.

You other intent was to set the value of mark based on the value of
classid. You _can do that today already_ with no changes via a policy in
user space. You suggested to do an ifdef so you wont have to type in the
line which says how to mark, and i said that was a bad idea (we need
less ifdefs not more). 

For the case of no actions compiled in:
nothing can write into the values of either tcindex or mark after
classification (on ingress), so it is ok to override. If you did this
for egress as well, that would be wrong because it is expected that some
qdiscs may set or utilize these metadatum.

I am not sure if it made more sense this time?

> What "result" are with:
> 1) no filters?
> 2) 1 filter only, with "action continue"?

Please refer to above verbosity and see if it all makes better sense.

cheers,
jamal

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[Dzianis Kahanovich]

Too many pixels to smoke. Sorry.

May be so? ;)) (if undefined classid not overwrited by random value tc_classify)
Even "tc" say to classid=0 - "????"

--- 1/net/sched/sch_ingress.c	2008-01-12 17:27:05.000000000 +0200
+++ 2/net/sched/sch_ingress.c	2008-01-22 22:09:32.000000000 +0200
@@ -136,6 +136,9 @@
  	struct ingress_qdisc_data *p = PRIV(sch);
  	struct tcf_result res;
  	int result;
+#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
+	res.classid=0;
+#endif

  	D2PRINTK("ingress_enqueue(skb %p,sch %p,[qdisc %p])\n", skb, sch, p);
  	result = tc_classify(skb, p->filter_list, &res);
@@ -169,6 +172,11 @@
  	sch->bstats.packets++;
  	sch->bstats.bytes += skb->len;
  #endif
+#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
+	if(res.classid)
+	    skb->mark = 
(skb->mark&(res.classid>>16))|(skb->tc_index=TC_H_MIN(res.classid));
+//	    skb->mark=res.classid; /* or just so */
+#endif

  	return result;
  }



jamal wrote:
> On Mon, 2008-14-01 at 20:20 -0200, Dzianis Kahanovich wrote:
>> jamal wrote:
> [..] 
> 
>>> Did that make sense?
>> After current "#endif" - may be.
> 
> I am afraid that would be counter to expected behavior. 
> Default is meant to apply when no value has been defined. Mark of 0 for
> example doesnt mean "default". Let me demonstrate with the ifdefs again
> with some arbitrary example:
> 
> -----------------
> #ifdef CONFIG_NET_CLS_ACT
> ..classify ...
>     .. action 1 sets mark to 0x11111
>     .. action 2 checks some state and conditionally let action 3 execute
>     .. action 3 sets mark to 0
> 
> if OK is returned set tc_index based on classid
> 
> #else // no actions compiled
> ..classify
> .... jamal suggests: set default mark and tc_index for ingress here
> #endif
> 
> // mahatma wants to set default for mark and tcindex here 
> // so it works for both actions and none-action code
> ------------------------
> 
> Lets look at the case of actions compiled in:
> I have defined my policies (in user space) so that the mark can be set
> to either 0 or 0x1111 depending on some runtime state. 
> Your default (kernel) code is now going to overide my policy - which is
> bad. Even in the case of OK being returned, it is wrong to set tc_index;
> unfortunately, we dont have an action that can set tc_index today; if we
> did, we would need to remove that setting.
> 
> You other intent was to set the value of mark based on the value of
> classid. You _can do that today already_ with no changes via a policy in
> user space. You suggested to do an ifdef so you wont have to type in the
> line which says how to mark, and i said that was a bad idea (we need
> less ifdefs not more). 
> 
> For the case of no actions compiled in:
> nothing can write into the values of either tcindex or mark after
> classification (on ingress), so it is ok to override. If you did this
> for egress as well, that would be wrong because it is expected that some
> qdiscs may set or utilize these metadatum.
> 
> I am not sure if it made more sense this time?
> 
>> What "result" are with:
>> 1) no filters?
>> 2) 1 filter only, with "action continue"?
> 
> Please refer to above verbosity and see if it all makes better sense.
> 
> cheers,
> jamal
> 
> 
> 
	

-- 
WBR,
Denis Kaganovich,  mahatma@eu.by  http://mahatma.bspu.unibel.by

Re: [PATCH 2.6.23+] ingress classify to [nf]mark

[Dzianis Kahanovich]

Too many pixels to smoke. Sorry.

May be so? ;)) (if undefined classid not overwrited by random value tc_classify)
Even "tc" say to classid=0 - "????"

--- 1/net/sched/sch_ingress.c	2008-01-12 17:27:05.000000000 +0200
+++ 2/net/sched/sch_ingress.c	2008-01-22 22:09:32.000000000 +0200
@@ -136,6 +136,9 @@
  	struct ingress_qdisc_data *p = PRIV(sch);
  	struct tcf_result res;
  	int result;
+#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
+	res.classid=0;
+#endif

  	D2PRINTK("ingress_enqueue(skb %p,sch %p,[qdisc %p])\n", skb, sch, p);
  	result = tc_classify(skb, p->filter_list, &res);
@@ -169,6 +172,11 @@
  	sch->bstats.packets++;
  	sch->bstats.bytes += skb->len;
  #endif
+#ifdef CONFIG_NET_SCH_INGRESS_TC2MARK
+	if(res.classid)
+	    skb->mark =
(skb->mark&(res.classid>>16))|(skb->tc_index=TC_H_MIN(res.classid));
+//	    skb->mark=res.classid; /* or just so */
+#endif

  	return result;
  }



jamal wrote:

[skipped]

-- 
WBR,
Denis Kaganovich,  mahatma@eu.by  http://mahatma.bspu.unibel.by