Message ID | CAEzD07+0EA12igqAcwbM8VGKLR9Hp0GjJ7t_Jve=JXOrEKvu9w@mail.gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | stephen hemminger |
Headers | show |
Hi Anton, On Sat, 2012-02-25 at 22:01 +0400, Anton 'EvilMan' Danilov wrote: > This patch adds the ability to set the mask for changing bits. The > mask parameter is optional. > > Example of usage: > #rewrite vlan id. cos field in 802.1q header is unchanged. > ip link add link eth0 name eth0.99 type vlan id 99 reorder_hdr off > egress-qos-map 0:7 > tc qdisc add dev eth0 root handle 1: prio bands 8 > tc filter add dev eth0 parent 1: protocol all pref 1 u32 > tc filter add dev eth0 parent 1: protocol all pref 1 handle ::1 u32 \ > match u16 0x8100 0xffff at -6 \ > match u16 0x0063 0x0fff at -4 \ > classid 1:5 \ > action pedit munge offset -4 u16 set 0x000d 0x0fff Looks like a reasonable feature to have - but it has to continue working as before for things that dont need the mask. > > diff --git a/tc/m_pedit.c b/tc/m_pedit.c > index 7499846..f081eff 100644 > --- a/tc/m_pedit.c > +++ b/tc/m_pedit.c > > tkey->val = htonl(tkey->val & retain); > - tkey->mask = htonl(tkey->mask | ~retain); > + tkey->mask = htonl(~tkey->mask | ~retain); A change like the above worries me that this may work for your use case but will break other things that used to work. I'd like to run some tests but dont have the cycles for the next few days. Alternatively, I could explain some tests to you and you could validate and fix whatever breaks. Let me know what is best for you. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Jamal. 2012/2/27 jamal <hadi@cyberus.ca>: > > Looks like a reasonable feature to have - but it has to continue working > as before for things that dont need the mask. > >> >> diff --git a/tc/m_pedit.c b/tc/m_pedit.c >> index 7499846..f081eff 100644 >> --- a/tc/m_pedit.c >> +++ b/tc/m_pedit.c > >> >> tkey->val = htonl(tkey->val & retain); >> - tkey->mask = htonl(tkey->mask | ~retain); >> + tkey->mask = htonl(~tkey->mask | ~retain); > > A change like the above worries me that this may work for your use case > but will break other things that used to work. I'd like to run some > tests but dont have the cycles for the next few days. Alternatively, I > could explain some tests to you and you could validate and fix whatever > breaks. > Let me know what is best for you. > Can You explain tests to me? I'll check behavior with my changes and fix breaks
Hi Anton, Thinking about this some more: Did you try the "retain" option? it is the xor mask to set operation. Examples: ---- #At offset 3 retain the first nibble but set the second to 0xA action pedit munge offset 3 u8 set 0xA0 retain 0xF #At offset 9, retain the second nibble but set the first to 0xA action pedit munge offset 9 u8 set 0x0A retain 0xF0 --- So if you were to point to the nibble holding the cos field in your match, then you can set it to a new value while retaining bitfields you dont want to change... I was going to ask you to test the retain in addition to the extra mask you are adding on different sets - but i am beginning to think the mask you are adding is not needed at all. Here is an example of other tests i was going to ask you to run (this time with 16bit field): #At offset 0, invert/clear/preserve the first 16 bits action pedit munge offset 0 u16 invert original data: 45000054 00004000 40013ca7 7f000001 modified data (invert): baff0054 00004000 40013ca7 7f000001 modified data (clear): 00000054 00004000 40013ca7 7f000001 retain 45000054 00004000 40013ca7 7f000001 set 0x1234 12340054 00004000 40013ca7 7f000001 You can pipe to a mirror action to send to dummy0 and then run tcpdump on dummy to watch the edited fields. I can describe a simple setup i use if this still doesnt make sense. As a side note: I think we need an action to work on VLANs since they are such an important field. The action would push/pop and edit VLAN fields. If you are interested in this, I can help provide guidance (It is one of those things on my todo longtimenow but i cant seem to get around to it). cheers, jamal cheers, jamal On Mon, 2012-02-27 at 17:15 +0300, Anton 'EvilMan' Danilov wrote: > Hi, Jamal. > > 2012/2/27 jamal <hadi@cyberus.ca>: > > > > Looks like a reasonable feature to have - but it has to continue working > > as before for things that dont need the mask. > > > >> > >> diff --git a/tc/m_pedit.c b/tc/m_pedit.c > >> index 7499846..f081eff 100644 > >> --- a/tc/m_pedit.c > >> +++ b/tc/m_pedit.c > > > >> > >> tkey->val = htonl(tkey->val & retain); > >> - tkey->mask = htonl(tkey->mask | ~retain); > >> + tkey->mask = htonl(~tkey->mask | ~retain); > > > > A change like the above worries me that this may work for your use case > > but will break other things that used to work. I'd like to run some > > tests but dont have the cycles for the next few days. Alternatively, I > > could explain some tests to you and you could validate and fix whatever > > breaks. > > Let me know what is best for you. > > > > Can You explain tests to me? I'll check behavior with my changes and fix breaks > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Jamal. 2012/2/28 Jamal Hadi Salim <jhs@mojatatu.com>: > > Thinking about this some more: > Did you try the "retain" option? it is the xor mask to set operation. > > I was going to ask you to test the retain in addition to the > extra mask you are adding on different sets - but i am beginning > to think the mask you are adding is not needed at all. > You are right, mask option isn't needed. Retain option must be used instead it. > > As a side note: I think we need an action to work on VLANs since > they are such an important field. The action would push/pop and edit > VLAN fields. If you are interested in this, I can help provide guidance > (It is one of those things on my todo longtimenow but i cant seem to get > around to it). > Action to work on Vlans is fine idea. I'm very interested in this action. I'll be grateful for any help.
Hi Anton, On Wed, 2012-02-29 at 16:12 +0300, Anton 'EvilMan' Danilov wrote: > > You are right, mask option isn't needed. Retain option must be used instead it. > Thanks for verifying. > Action to work on Vlans is fine idea. I'm very interested in this > action. I'll be grateful for any help. Lets start by defining the interface to be seen by user space such as vlan editing/popping/pushing etc. Then i will help you build it slowly until you are comfortable with moving with it. As a matter of fact, lets talk offline so we dont add more noise to the list; you can always post the RFC to the list when you are ready. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 29 Feb 2012 09:01:20 -0500 jamal <hadi@cyberus.ca> wrote: > Hi Anton, > > On Wed, 2012-02-29 at 16:12 +0300, Anton 'EvilMan' Danilov wrote: > > > > > You are right, mask option isn't needed. Retain option must be used instead it. > > > > Thanks for verifying. > > > Action to work on Vlans is fine idea. I'm very interested in this > > action. I'll be grateful for any help. > > Lets start by defining the interface to be seen by user space > such as vlan editing/popping/pushing etc. Then i will help you > build it slowly until you are comfortable with moving with it. > > As a matter of fact, lets talk offline so we dont add more noise > to the list; you can always post the RFC to the list when you are > ready. > > cheers, > jamal Did you come to a conclusion, should it be applied as is? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-04-25 at 13:15 -0700, Stephen Hemminger wrote:
> Did you come to a conclusion, should it be applied as is?
No, dont apply it as is please.
EvilMan should have a new version when he gets to it.
cheers,
jamal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tc/m_pedit.c b/tc/m_pedit.c index 7499846..f081eff 100644 --- a/tc/m_pedit.c +++ b/tc/m_pedit.c @@ -44,7 +44,8 @@ explain(void) "\t\tNOTE: offval is byte offset, must be multiple of 4\n " "\t\tNOTE: maskval is a 32 bit hex number\n " "\t\tNOTE: shiftval is a is a shift value\n " - "\t\tCMD:= clear | invert | set <setval>| retain\n " + "\t\tCMD:= clear [<mask>] | invert [<mask>]\n" + " \t\t| set <setval> [<mask>] | retain [<mask>] \n " "\t<LAYERED>:= ip <ipdata> | ip6 <ip6data> \n " " \t\t| udp <udpdata> | tcp <tcpdata> | icmp <icmpdata> \n" "For Example usage look at the examples directory\n"); @@ -152,7 +153,7 @@ pack_key32(__u32 retain,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey) } tkey->val = htonl(tkey->val & retain); - tkey->mask = htonl(tkey->mask | ~retain); + tkey->mask = htonl(~tkey->mask | ~retain); /* jamal remove this - it is not necessary given the if check above */ tkey->off &= ~3;