| Message ID | 88b3c1de-b11c-ee9b-e251-43e1ac47592a@solarflare.com |
|---|---|
| Headers | show |
| Series | flow_offload: Re-add per-actionstatistics | expand |
On 15/05/2019 20:39, Edward Cree wrote: > A point for discussion: would it be better if, instead of the tcfa_index > (for which the driver has to know the rules about which flow_action > types share a namespace), we had some kind of globally unique cookie? > In the same way that rule->cookie is really a pointer, could we use the > address of the TC-internal data structure representing the action? Do > rules that share an action all point to the same struct tc_action in > their tcf_exts, for instance? A quick test showed that, indeed, they do; I'm now leaning towards the approach of adding "unsigned long cookie" to struct flow_action_entry and populating it with (unsigned long)act in tc_setup_flow_action(). Pablo, how do the two options interact with your netfilter offload? I'm guessing it's easier for you to find a unique pointer than to generate a unique u32 action_index for each action. I'm also assuming that netfilter doesn't have a notion of shared actions. -Ed
On 17/05/2019 16:27, Edward Cree wrote: > I'm now leaning towards the > approach of adding "unsigned long cookie" to struct flow_action_entry > and populating it with (unsigned long)act in tc_setup_flow_action(). For concreteness, here's what that looks like: patch 1 is replaced with the following, the other two are unchanged. Drivers now have an easier job, as they can just use the cookie directly as a hashtable key, rather than worrying about which action types share indices. --8<-- [RFC PATCH v2.5 net-next 1/3] flow_offload: add a cookie to flow_action_entry Populated with the address of the struct tc_action from which it was made. Required for support of shared counters (and possibly other shared per- action entities in future). Signed-off-by: Edward Cree <ecree@solarflare.com> --- include/net/flow_offload.h | 1 + net/sched/cls_api.c | 1 + 2 files changed, 2 insertions(+) diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index 6200900434e1..fb3278a2bd41 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -137,6 +137,7 @@ enum flow_action_mangle_base { struct flow_action_entry { enum flow_action_id id; + unsigned long cookie; union { u32 chain_index; /* FLOW_ACTION_GOTO */ struct net_device *dev; /* FLOW_ACTION_REDIRECT */ diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index d4699156974a..5411cec17af5 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -3195,6 +3195,7 @@ int tc_setup_flow_action(struct flow_action *flow_action, struct flow_action_entry *entry; entry = &flow_action->entries[j]; + entry->cookie = (unsigned long)act; if (is_tcf_gact_ok(act)) { entry->id = FLOW_ACTION_ACCEPT; } else if (is_tcf_gact_shot(act)) {
On 2019-05-15 3:39 p.m., Edward Cree wrote: [..] > > A point for discussion: would it be better if, instead of the tcfa_index > (for which the driver has to know the rules about which flow_action > types share a namespace), we had some kind of globally unique cookie? > In the same way that rule->cookie is really a pointer, could we use the > address of the TC-internal data structure representing the action? tcfa_index + action identifier seem to be sufficiently global, no? Then we dont have a mismatch with what the kernel(non-offloaded) semantics. Note: The kernel is free to generate the index (if the user doesnt specify). > Do > rules that share an action all point to the same struct tc_action in > their tcf_exts, for instance? Yes they do. cheers, jamal
On 2019-05-17 1:14 p.m., Edward Cree wrote: > On 17/05/2019 16:27, Edward Cree wrote: >> I'm now leaning towards the >> approach of adding "unsigned long cookie" to struct flow_action_entry >> and populating it with (unsigned long)act in tc_setup_flow_action(). > > For concreteness, here's what that looks like: patch 1 is replaced with > the following, the other two are unchanged. > Drivers now have an easier job, as they can just use the cookie directly > as a hashtable key, rather than worrying about which action types share > indices. Per my other email, this will break tc semantics. It doesnt look possible to specify an index from user space. Did i miss something? cheers, jamal
On Fri, May 17, 2019 at 04:27:29PM +0100, Edward Cree wrote: > On 15/05/2019 20:39, Edward Cree wrote: [...] > Pablo, how do the two options interact with your netfilter offload? I'm > guessing it's easier for you to find a unique pointer than to generate > a unique u32 action_index for each action. I'm also assuming that > netfilter doesn't have a notion of shared actions. It has that shared actions concept, see: https://netfilter.org/projects/nfacct/ Have a look at 'nfacct' in iptables-extensions(8) manpage.
On 18/05/2019 21:39, Jamal Hadi Salim wrote: > On 2019-05-17 1:14 p.m., Edward Cree wrote: >> On 17/05/2019 16:27, Edward Cree wrote: >>> I'm now leaning towards the >>> approach of adding "unsigned long cookie" to struct flow_action_entry >>> and populating it with (unsigned long)act in tc_setup_flow_action(). >> >> For concreteness, here's what that looks like: patch 1 is replaced with >> the following, the other two are unchanged. >> Drivers now have an easier job, as they can just use the cookie directly >> as a hashtable key, rather than worrying about which action types share >> indices. > > Per my other email, this will break tc semantics. It doesnt look > possible to specify an index from user space. Did i miss > something? Unless *I* missed something, I'm not changing the TC<=>user-space API at all. If user space specifies an index, then TC will either create a new action with that index, or find an existing one. Then flow_offload turns that into a cookie; in the 'existing action' case it'll be the same cookie as any previous offloads of that action, in the 'new action' case it'll be a cookie distinct from any existing action. Drivers aren't interested in the specific index value, only in "which other actions (counters) I've offloaded are shared with this one?", which the cookie gives them. With my (unreleased) driver code, I've successfully tested this with e.g. the following rules: tc filter add dev $vfrep parent ffff: protocol arp flower skip_sw \ action vlan push id 100 protocol 802.1q \ action mirred egress mirror dev $pf index 101 \ action vlan pop \ action drop index 104 tc filter add dev $vfrep parent ffff: protocol ipv4 flower skip_sw \ action vlan push id 100 protocol 802.1q \ action mirred egress mirror dev $pf index 102 \ action vlan pop \ action drop index 104 Then when viewing with `tc -stats filter show`, the mirreds count their traffic separately (and with an extra 4 bytes per packet for the VLAN), whereas the drops (index 104, shared) show the total count (and without the 4 bytes). (From your other email) > tcfa_index + action identifier seem to be sufficiently global, no? The reason I don't like using the action identifier is because flow_offload slightly alters those: mirred gets split into two (FLOW_ACTION_REDIRECT and FLOW_ACTION_MIRRED (mirror)). Technically it'll still work (a redirect and a mirror are different actions, so can't have the same index, so it doesn't matter if they're treated as the same action-type or not) but it feels like a kludge. -Ed
On 19/05/2019 01:22, Pablo Neira Ayuso wrote: > On Fri, May 17, 2019 at 04:27:29PM +0100, Edward Cree wrote: >> On 15/05/2019 20:39, Edward Cree wrote: > [...] >> Pablo, how do the two options interact with your netfilter offload? I'm >> guessing it's easier for you to find a unique pointer than to generate >> a unique u32 action_index for each action. I'm also assuming that >> netfilter doesn't have a notion of shared actions. > It has that shared actions concept, see: > > https://netfilter.org/projects/nfacct/ > > Have a look at 'nfacct' in iptables-extensions(8) manpage. Thanks. Looking at net/netfilter/nfnetlink_acct.c, it looks as though you don't have a u32 index in there; for the cookie approach, would the address of the struct nf_acct (casted to unsigned long) work to uniquely identify actions that should be shared? I'm not 100% sure how nf (or nfacct) offload is going to look, so I might be barking up the wrong tree here. But it seems like the cookie method should work better for you — even if you did have an index, how would you avoid collisions with TC actions using the same indices if both are in use on a box? Cookies OTOH are pointers, so guaranteed unique :) -Ed
On 2019-05-20 11:26 a.m., Edward Cree wrote: > On 18/05/2019 21:39, Jamal Hadi Salim wrote: >> On 2019-05-17 1:14 p.m., Edward Cree wrote: >>> On 17/05/2019 16:27, Edward Cree wrote: > Unless *I* missed something, I'm not changing the TC<=>user-space API at > all. If user space specifies an index, then TC will either create a new > action with that index, or find an existing one. Then flow_offload turns > that into a cookie; in the 'existing action' case it'll be the same > cookie as any previous offloads of that action, in the 'new action' case > it'll be a cookie distinct from any existing action. That is fine then if i could do: tc actions add action drop index 104 then followed by for example the two filters you show below.. Is your hardware not using explicit indices into a stats table? > Drivers aren't interested in the specific index value, only in "which > other actions (counters) I've offloaded are shared with this one?", which > the cookie gives them. > > With my (unreleased) driver code, I've successfully tested this with e.g. > the following rules: > tc filter add dev $vfrep parent ffff: protocol arp flower skip_sw \ > action vlan push id 100 protocol 802.1q \ > action mirred egress mirror dev $pf index 101 \ > action vlan pop \ > action drop index 104 > tc filter add dev $vfrep parent ffff: protocol ipv4 flower skip_sw \ > action vlan push id 100 protocol 802.1q \ > action mirred egress mirror dev $pf index 102 \ > action vlan pop \ > action drop index 104 > > Then when viewing with `tc -stats filter show`, the mirreds count their > traffic separately (and with an extra 4 bytes per packet for the VLAN), > whereas the drops (index 104, shared) show the total count (and without > the 4 bytes). > Beauty. Assuming the stats are being synced to the kernel? Test 1: What does "tc -s actions ls action drop index 104" show? Test 2: Delete one of the filters above then dump actions again as above. > (From your other email) >> tcfa_index + action identifier seem to be sufficiently global, no? > The reason I don't like using the action identifier is because flow_offload > slightly alters those: mirred gets split into two (FLOW_ACTION_REDIRECT > and FLOW_ACTION_MIRRED (mirror)). Technically it'll still work (a redirect > and a mirror are different actions, so can't have the same index, so it > doesn't matter if they're treated as the same action-type or not) but it > feels like a kludge. > As long as uapi semantics are not broken (which you are demonstrating it is not) then we are good. cheers, jamal > -Ed >
On 2019-05-20 11:37 a.m., Edward Cree wrote: > On 19/05/2019 01:22, Pablo Neira Ayuso wrote: >> On Fri, May 17, 2019 at 04:27:29PM +0100, Edward Cree wrote: > Thanks. Looking at net/netfilter/nfnetlink_acct.c, it looks as though you > don't have a u32 index in there; for the cookie approach, would the > address of the struct nf_acct (casted to unsigned long) work to uniquely > identify actions that should be shared? > I'm not 100% sure how nf (or nfacct) offload is going to look, so I might > be barking up the wrong tree here. But it seems like the cookie method > should work better for you — even if you did have an index, how would you > avoid collisions with TC actions using the same indices if both are in > use on a box? Cookies OTOH are pointers, so guaranteed unique :) A little concerned: Hopefully all these can be manipulated by tc as well - otherwise we are opening some other big pandora box of two subsystems fighting each other. cheers, jamal
On Mon, May 20, 2019 at 04:37:10PM +0100, Edward Cree wrote: > On 19/05/2019 01:22, Pablo Neira Ayuso wrote: > > On Fri, May 17, 2019 at 04:27:29PM +0100, Edward Cree wrote: > >> On 15/05/2019 20:39, Edward Cree wrote: > > [...] > >> Pablo, how do the two options interact with your netfilter offload? I'm > >> guessing it's easier for you to find a unique pointer than to generate > >> a unique u32 action_index for each action. I'm also assuming that > >> netfilter doesn't have a notion of shared actions. > > It has that shared actions concept, see: > > > > https://netfilter.org/projects/nfacct/ > > > > Have a look at 'nfacct' in iptables-extensions(8) manpage. > > Thanks. Looking at net/netfilter/nfnetlink_acct.c, it looks as though you > don't have a u32 index in there; for the cookie approach, would the > address of the struct nf_acct (casted to unsigned long) work to uniquely > identify actions that should be shared? > I'm not 100% sure how nf (or nfacct) offload is going to look, so I might > be barking up the wrong tree here. But it seems like the cookie method > should work better for you — even if you did have an index, how would you > avoid collisions with TC actions using the same indices if both are in > use on a box? Cookies OTOH are pointers, so guaranteed unique :) The cookie approach per-action looks fine to me, there's already a cookie to identify the rule, so this looks natural to me.
On 20/05/2019 16:38, Jamal Hadi Salim wrote: > That is fine then if i could do: > > tc actions add action drop index 104 > then > followed by for example the two filters you show below.. That seems to work. > Is your hardware not using explicit indices into a stats table? No; we ask the HW to allocate a counter and it returns us a counter ID (which bears no relation to the action index). So I have an rhashtable keyed on the cookie (or on the action-type & action_index, when using the other version of my patches) which stores the HW counter ID; and the entry in that hashtable is what I attach to the driver's action struct. > Beauty. Assuming the stats are being synced to the kernel? > Test 1: > What does "tc -s actions ls action drop index 104" show? It produces no output, but `tc -s actions get action drop index 104` or `tc -s actions list action gact index 104` shows the same stats as `tc -s filter show ...` did for that action. > Test 2: > Delete one of the filters above then dump actions again as above. Ok, that's weird: after I delete one, the other (in `tc -s filter show ...`) no longer shows the shared action. # tc filter del dev $vfrep parent ffff: pref 49151 # tc -stats filter show dev $vfrep parent ffff: filter protocol arp pref 49152 flower chain 0 filter protocol arp pref 49152 flower chain 0 handle 0x1 eth_type arp skip_sw in_hw in_hw_count 1 action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe index 1 ref 1 bind 1 installed 180 sec used 180 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 2: mirred (Egress Mirror to device $pf) pipe index 101 ref 1 bind 1 installed 180 sec used 169 sec Action statistics: Sent 256 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 256 bytes 4 pkt backlog 0b 0p requeues 0 action order 3: vlan pop pipe index 2 ref 1 bind 1 installed 180 sec used 180 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 # Yet `tc -s actions get` still shows it... # tc -s actions get action drop index 104 total acts 0 action order 1: gact action drop random type none pass val 0 index 104 ref 2 bind 1 installed 812 sec used 797 sec Action statistics: Sent 534 bytes 7 pkt (dropped 7, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 534 bytes 7 pkt backlog 0b 0p requeues 0 # tc filter show dev $vfrep parent ffff: filter protocol arp pref 49152 flower chain 0 filter protocol arp pref 49152 flower chain 0 handle 0x1 eth_type arp skip_sw in_hw in_hw_count 1 action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe index 1 ref 1 bind 1 action order 3: vlan pop pipe index 2 ref 1 bind 1 # tc -s actions get action mirred index 101 total acts 0 action order 1: mirred (Egress Mirror to device $pf) pipe index 101 ref 1 bind 1 installed 796 sec used 785 sec Action statistics: Sent 256 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 256 bytes 4 pkt backlog 0b 0p requeues 0 # Curiouser and curiouser... it seems that after I delete one of the rules, TC starts to get very confused and actions start disappearing from rule dumps. Yet those actions still exist according to `tc actions list`. I don't *think* my changes can have caused this, but I'll try a test on a vanilla kernel just to make sure the same thing happens there. -Ed
On 2019-05-20 12:10 p.m., Edward Cree wrote: > On 20/05/2019 16:38, Jamal Hadi Salim wrote: >> That is fine then if i could do: >> >> tc actions add action drop index 104 >> then >> followed by for example the two filters you show below.. > That seems to work. nice. >> Is your hardware not using explicit indices into a stats table? > No; we ask the HW to allocate a counter and it returns us a counter ID (which > bears no relation to the action index). So I have an rhashtable keyed on > the cookie (or on the action-type & action_index, when using the other > version of my patches) which stores the HW counter ID; and the entry in that > hashtable is what I attach to the driver's action struct. > >> Beauty. Assuming the stats are being synced to the kernel? >> Test 1: >> What does "tc -s actions ls action drop index 104" show? > It produces no output, but > `tc -s actions get action drop index 104` > or > `tc -s actions list action gact index 104` sorry meant that. Hopefully it shows accumulated stats from both filter hits and correct ref counts and bind counts. > shows the same stats as `tc -s filter show ...` did for that action. >> Test 2: >> Delete one of the filters above then dump actions again as above. > Ok, that's weird: after I delete one, the other (in `tc -s filter show ...`) > no longer shows the shared action. > Sounds weird. > # tc filter del dev $vfrep parent ffff: pref 49151 > # tc -stats filter show dev $vfrep parent ffff: > filter protocol arp pref 49152 flower chain 0 > filter protocol arp pref 49152 flower chain 0 handle 0x1 > eth_type arp > skip_sw > in_hw in_hw_count 1 > action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe > index 1 ref 1 bind 1 installed 180 sec used 180 sec > Action statistics: > Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) > backlog 0b 0p requeues 0 > > action order 2: mirred (Egress Mirror to device $pf) pipe > index 101 ref 1 bind 1 installed 180 sec used 169 sec > Action statistics: > Sent 256 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) > Sent software 0 bytes 0 pkt > Sent hardware 256 bytes 4 pkt > backlog 0b 0p requeues 0 > > action order 3: vlan pop pipe > index 2 ref 1 bind 1 installed 180 sec used 180 sec > Action statistics: > Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) > backlog 0b 0p requeues 0 > > # > Hold on, did you intentionaly add that "protocol arp" there? Or is that just a small development time quark? > Yet `tc -s actions get` still shows it... > > # tc -s actions get action drop index 104 > total acts 0 > > action order 1: gact action drop > random type none pass val 0 > index 104 ref 2 bind 1 installed 812 sec used 797 sec > Action statistics: > Sent 534 bytes 7 pkt (dropped 7, overlimits 0 requeues 0) > Sent software 0 bytes 0 pkt > Sent hardware 534 bytes 7 pkt > backlog 0b 0p requeues 0 Assuming you first created the action then bound it, the action output looks correct. "ref 2" implies two refcounts, one by the first action creation and the second by the filter binding. Also "bind 1" implies only one filter is referencing it. Before you deleted it should have been "ref 3" and "bind 2". > # tc filter show dev $vfrep parent ffff: > filter protocol arp pref 49152 flower chain 0 > filter protocol arp pref 49152 flower chain 0 handle 0x1 > eth_type arp > skip_sw > in_hw in_hw_count 1 > action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe > index 1 ref 1 bind 1 > > action order 3: vlan pop pipe > index 2 ref 1 bind 1 I see the arp listing again. Maybe just a bug. > # tc -s actions get action mirred index 101 > total acts 0 > > action order 1: mirred (Egress Mirror to device $pf) pipe > index 101 ref 1 bind 1 installed 796 sec used 785 sec > Action statistics: > Sent 256 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) > Sent software 0 bytes 0 pkt > Sent hardware 256 bytes 4 pkt > backlog 0b 0p requeues 0 > # Assuming in this case you added by value the actions? Bind and ref are 1 each. Maybe dump after each step and it will be easier to see what is happening. > Curiouser and curiouser... it seems that after I delete one of the rules, > TC starts to get very confused and actions start disappearing from rule > dumps. Yet those actions still exist according to `tc actions list`. > I don't *think* my changes can have caused this, but I'll try a test on a > vanilla kernel just to make sure the same thing happens there. > Possible an offload bug that was already in existence. Can you try the same steps but without offloading and see if you see the same behavior? cheers, jamal
On 2019-05-20 12:29 p.m., Jamal Hadi Salim wrote:
> Assuming in this case you added by value the actions?
To be clear on the terminology:
"By Value" implies you add the filter and action in the
same command line.
"By Reference" implies you first create the action then
create a filter which binds to the already created action.
cheers,
jamal
On 20/05/2019 17:29, Jamal Hadi Salim wrote: > On 2019-05-20 12:10 p.m., Edward Cree wrote: >> # tc filter del dev $vfrep parent ffff: pref 49151 >> # tc -stats filter show dev $vfrep parent ffff: >> filter protocol arp pref 49152 flower chain 0 >> filter protocol arp pref 49152 flower chain 0 handle 0x1 >> eth_type arp >> skip_sw >> in_hw in_hw_count 1 >> action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe >> index 1 ref 1 bind 1 installed 180 sec used 180 sec >> Action statistics: >> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) >> backlog 0b 0p requeues 0 >> >> action order 2: mirred (Egress Mirror to device $pf) pipe >> index 101 ref 1 bind 1 installed 180 sec used 169 sec >> Action statistics: >> Sent 256 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) >> Sent software 0 bytes 0 pkt >> Sent hardware 256 bytes 4 pkt >> backlog 0b 0p requeues 0 >> >> action order 3: vlan pop pipe >> index 2 ref 1 bind 1 installed 180 sec used 180 sec >> Action statistics: >> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) >> backlog 0b 0p requeues 0 >> >> # >> > > Hold on, did you intentionaly add that "protocol arp" there? Yes; if you go back to my `tc filter add` commands, I had one matching 'protocol arp' and the other 'protocol ipv4', and the latter is the one I then deleted. I'm not 100% sure why `tc filter show` prints it twice ('protocol' and 'eth_type'), though. >> # tc -s actions get action mirred index 101 >> total acts 0 >> >> action order 1: mirred (Egress Mirror to device $pf) pipe >> index 101 ref 1 bind 1 installed 796 sec used 785 sec >> Action statistics: >> Sent 256 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) >> Sent software 0 bytes 0 pkt >> Sent hardware 256 bytes 4 pkt >> backlog 0b 0p requeues 0 >> # > > Assuming in this case you added by value the actions? Bind and ref > are 1 each. Yes, the mirreds were added by value. > Possible an offload bug that was already in existence. Can you try the > same steps but without offloading and see if you see the same behavior? Running with 'skip_hw' instead of 'skip_sw', I *don't* see the same behaviour. -Ed
On 20/05/2019 17:29, Jamal Hadi Salim wrote: > Maybe dump after each step and it will be easier to see what is > happening. > >> I don't *think* my changes can have caused this, but I'll try a test on a >> vanilla kernel just to make sure the same thing happens there. > > Possible an offload bug that was already in existence. I've now reproduced on vanilla net-next (63863ee8e2f6); the breakage occurs when I run `tc -s actions get action drop index 104`, even _without_ having previously deleted a rule. And in a correction to my last email, it turns out this *does* reproduce without offloading (I evidently didn't hit the right sequence to tickle the bug before). # tc -stats filter show dev $vfrep parent ffff: filter protocol ip pref 49151 flower chain 0 filter protocol ip pref 49151 flower chain 0 handle 0x1 eth_type ipv4 skip_sw in_hw in_hw_count 1 action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe index 3 ref 1 bind 1 installed 7 sec used 7 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 2: mirred (Egress Mirror to device $pf) pipe index 102 ref 1 bind 1 installed 7 sec used 3 sec Action statistics: Sent 306 bytes 3 pkt (dropped 0, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 306 bytes 3 pkt backlog 0b 0p requeues 0 action order 3: vlan pop pipe index 4 ref 1 bind 1 installed 7 sec used 7 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 4: gact action drop random type none pass val 0 index 104 ref 3 bind 2 installed 13 sec used 3 sec Action statistics: Sent 306 bytes 3 pkt (dropped 3, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 306 bytes 3 pkt backlog 0b 0p requeues 0 filter protocol arp pref 49152 flower chain 0 filter protocol arp pref 49152 flower chain 0 handle 0x1 eth_type arp skip_sw in_hw in_hw_count 1 action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe index 1 ref 1 bind 1 installed 7 sec used 7 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 2: mirred (Egress Mirror to device $pf) pipe index 101 ref 1 bind 1 installed 7 sec used 5 sec Action statistics: Sent 64 bytes 1 pkt (dropped 0, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 64 bytes 1 pkt backlog 0b 0p requeues 0 action order 3: vlan pop pipe index 2 ref 1 bind 1 installed 7 sec used 7 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 4: gact action drop random type none pass val 0 index 104 ref 3 bind 2 installed 13 sec used 3 sec Action statistics: Sent 370 bytes 4 pkt (dropped 4, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 370 bytes 4 pkt backlog 0b 0p requeues 0 # tc -s actions get action drop index 104 total acts 0 action order 1: gact action drop random type none pass val 0 index 104 ref 3 bind 2 installed 27 sec used 17 sec Action statistics: Sent 370 bytes 4 pkt (dropped 4, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 370 bytes 4 pkt backlog 0b 0p requeues 0 # tc -stats filter show dev $vfrep parent ffff: filter protocol ip pref 49151 flower chain 0 filter protocol ip pref 49151 flower chain 0 handle 0x1 eth_type ipv4 skip_sw in_hw in_hw_count 1 action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe index 3 ref 1 bind 1 installed 26 sec used 26 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 2: mirred (Egress Mirror to device $pf) pipe index 102 ref 1 bind 1 installed 26 sec used 22 sec Action statistics: Sent 306 bytes 3 pkt (dropped 0, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 306 bytes 3 pkt backlog 0b 0p requeues 0 action order 3: vlan pop pipe index 4 ref 1 bind 1 installed 26 sec used 26 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 filter protocol arp pref 49152 flower chain 0 filter protocol arp pref 49152 flower chain 0 handle 0x1 eth_type arp skip_sw in_hw in_hw_count 1 action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe index 1 ref 1 bind 1 installed 27 sec used 27 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 2: mirred (Egress Mirror to device $pf) pipe index 101 ref 1 bind 1 installed 27 sec used 17 sec Action statistics: Sent 256 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 256 bytes 4 pkt backlog 0b 0p requeues 0 action order 3: vlan pop pipe index 2 ref 1 bind 1 installed 27 sec used 27 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 #
On 2019-05-20 2:36 p.m., Edward Cree wrote: > On 20/05/2019 17:29, Jamal Hadi Salim wrote: >> Maybe dump after each step and it will be easier to see what is >> happening. >> >>> I don't *think* my changes can have caused this, but I'll try a test on a >>> vanilla kernel just to make sure the same thing happens there. >> >> Possible an offload bug that was already in existence. > I've now reproduced on vanilla net-next (63863ee8e2f6); the breakage occurs > when I run `tc -s actions get action drop index 104`, even _without_ having > previously deleted a rule. > And in a correction to my last email, it turns out this *does* reproduce > without offloading (I evidently didn't hit the right sequence to tickle > the bug before). > Ok, so the "get" does it. Will try to reproduce when i get some cycles. Meantime CCing Cong and Vlad. cheers, jamal > # tc -stats filter show dev $vfrep parent ffff: > filter protocol ip pref 49151 flower chain 0 > filter protocol ip pref 49151 flower chain 0 handle 0x1 > eth_type ipv4 > skip_sw > in_hw in_hw_count 1 > action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe > index 3 ref 1 bind 1 installed 7 sec used 7 sec > Action statistics: > Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) > backlog 0b 0p requeues 0 > > action order 2: mirred (Egress Mirror to device $pf) pipe > index 102 ref 1 bind 1 installed 7 sec used 3 sec > Action statistics: > Sent 306 bytes 3 pkt (dropped 0, overlimits 0 requeues 0) > Sent software 0 bytes 0 pkt > Sent hardware 306 bytes 3 pkt > backlog 0b 0p requeues 0 > > action order 3: vlan pop pipe > index 4 ref 1 bind 1 installed 7 sec used 7 sec > Action statistics: > Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) > backlog 0b 0p requeues 0 > > action order 4: gact action drop > random type none pass val 0 > index 104 ref 3 bind 2 installed 13 sec used 3 sec > Action statistics: > Sent 306 bytes 3 pkt (dropped 3, overlimits 0 requeues 0) > Sent software 0 bytes 0 pkt > Sent hardware 306 bytes 3 pkt > backlog 0b 0p requeues 0 > > filter protocol arp pref 49152 flower chain 0 > filter protocol arp pref 49152 flower chain 0 handle 0x1 > eth_type arp > skip_sw > in_hw in_hw_count 1 > action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe > index 1 ref 1 bind 1 installed 7 sec used 7 sec > Action statistics: > Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) > backlog 0b 0p requeues 0 > > action order 2: mirred (Egress Mirror to device $pf) pipe > index 101 ref 1 bind 1 installed 7 sec used 5 sec > Action statistics: > Sent 64 bytes 1 pkt (dropped 0, overlimits 0 requeues 0) > Sent software 0 bytes 0 pkt > Sent hardware 64 bytes 1 pkt > backlog 0b 0p requeues 0 > > action order 3: vlan pop pipe > index 2 ref 1 bind 1 installed 7 sec used 7 sec > Action statistics: > Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) > backlog 0b 0p requeues 0 > > action order 4: gact action drop > random type none pass val 0 > index 104 ref 3 bind 2 installed 13 sec used 3 sec > Action statistics: > Sent 370 bytes 4 pkt (dropped 4, overlimits 0 requeues 0) > Sent software 0 bytes 0 pkt > Sent hardware 370 bytes 4 pkt > backlog 0b 0p requeues 0 > > # tc -s actions get action drop index 104 > total acts 0 > > action order 1: gact action drop > random type none pass val 0 > index 104 ref 3 bind 2 installed 27 sec used 17 sec > Action statistics: > Sent 370 bytes 4 pkt (dropped 4, overlimits 0 requeues 0) > Sent software 0 bytes 0 pkt > Sent hardware 370 bytes 4 pkt > backlog 0b 0p requeues 0 > # tc -stats filter show dev $vfrep parent ffff: > filter protocol ip pref 49151 flower chain 0 > filter protocol ip pref 49151 flower chain 0 handle 0x1 > eth_type ipv4 > skip_sw > in_hw in_hw_count 1 > action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe > index 3 ref 1 bind 1 installed 26 sec used 26 sec > Action statistics: > Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) > backlog 0b 0p requeues 0 > > action order 2: mirred (Egress Mirror to device $pf) pipe > index 102 ref 1 bind 1 installed 26 sec used 22 sec > Action statistics: > Sent 306 bytes 3 pkt (dropped 0, overlimits 0 requeues 0) > Sent software 0 bytes 0 pkt > Sent hardware 306 bytes 3 pkt > backlog 0b 0p requeues 0 > > action order 3: vlan pop pipe > index 4 ref 1 bind 1 installed 26 sec used 26 sec > Action statistics: > Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) > backlog 0b 0p requeues 0 > > filter protocol arp pref 49152 flower chain 0 > filter protocol arp pref 49152 flower chain 0 handle 0x1 > eth_type arp > skip_sw > in_hw in_hw_count 1 > action order 1: vlan push id 100 protocol 802.1Q priority 0 pipe > index 1 ref 1 bind 1 installed 27 sec used 27 sec > Action statistics: > Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) > backlog 0b 0p requeues 0 > > action order 2: mirred (Egress Mirror to device $pf) pipe > index 101 ref 1 bind 1 installed 27 sec used 17 sec > Action statistics: > Sent 256 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) > Sent software 0 bytes 0 pkt > Sent hardware 256 bytes 4 pkt > backlog 0b 0p requeues 0 > > action order 3: vlan pop pipe > index 2 ref 1 bind 1 installed 27 sec used 27 sec > Action statistics: > Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) > backlog 0b 0p requeues 0 > > # >
On 2019-05-20 5:12 p.m., Jamal Hadi Salim wrote: > On 2019-05-20 2:36 p.m., Edward Cree wrote: >> On 20/05/2019 17:29, Jamal Hadi Salim wrote: > Ok, so the "get" does it. Will try to reproduce when i get some > cycles. Meantime CCing Cong and Vlad. > I have reproduced it in a simpler setup. See attached. Vlad this is likely from your changes. Sorry no cycles to dig more. Lucas, can we add this to the testcases? cheers, jamal sudo tc qdisc del dev lo ingress sudo tc qdisc add dev lo ingress sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ match ip dst 127.0.0.8/32 flowid 1:10 \ action vlan push id 100 protocol 802.1q \ action drop index 104 sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ match ip dst 127.0.0.10/32 flowid 1:10 \ action vlan push id 101 protocol 802.1q \ action drop index 104 # sudo tc -s filter ls dev lo parent ffff: protocol ip #this will now delete action gact index 104(drop) from display sudo tc -s actions get action drop index 104 sudo tc -s filter ls dev lo parent ffff: protocol ip #But you can still see it if you do this: sudo tc -s actions get action drop index 104
On Tue 21 May 2019 at 15:46, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 2019-05-20 5:12 p.m., Jamal Hadi Salim wrote: >> On 2019-05-20 2:36 p.m., Edward Cree wrote: >>> On 20/05/2019 17:29, Jamal Hadi Salim wrote: > >> Ok, so the "get" does it. Will try to reproduce when i get some >> cycles. Meantime CCing Cong and Vlad. >> > > > I have reproduced it in a simpler setup. See attached. Vlad this is > likely from your changes. Sorry no cycles to dig more. Jamal, thanks for minimizing the reproduction. I'll look into it. > Lucas, can we add this to the testcases? > > > cheers, > jamal > > sudo tc qdisc del dev lo ingress > sudo tc qdisc add dev lo ingress > > sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ > match ip dst 127.0.0.8/32 flowid 1:10 \ > action vlan push id 100 protocol 802.1q \ > action drop index 104 > > sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ > match ip dst 127.0.0.10/32 flowid 1:10 \ > action vlan push id 101 protocol 802.1q \ > action drop index 104 > > # > sudo tc -s filter ls dev lo parent ffff: protocol ip > > #this will now delete action gact index 104(drop) from display > sudo tc -s actions get action drop index 104 > > sudo tc -s filter ls dev lo parent ffff: protocol ip > > #But you can still see it if you do this: > sudo tc -s actions get action drop index 104
On Tue 21 May 2019 at 16:23, Vlad Buslov <vladbu@mellanox.com> wrote: > On Tue 21 May 2019 at 15:46, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> On 2019-05-20 5:12 p.m., Jamal Hadi Salim wrote: >>> On 2019-05-20 2:36 p.m., Edward Cree wrote: >>>> On 20/05/2019 17:29, Jamal Hadi Salim wrote: >> >>> Ok, so the "get" does it. Will try to reproduce when i get some >>> cycles. Meantime CCing Cong and Vlad. >>> >> >> >> I have reproduced it in a simpler setup. See attached. Vlad this is >> likely from your changes. Sorry no cycles to dig more. > > Jamal, thanks for minimizing the reproduction. I'll look into it. > >> Lucas, can we add this to the testcases? >> >> >> cheers, >> jamal >> >> sudo tc qdisc del dev lo ingress >> sudo tc qdisc add dev lo ingress >> >> sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ >> match ip dst 127.0.0.8/32 flowid 1:10 \ >> action vlan push id 100 protocol 802.1q \ >> action drop index 104 >> >> sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ >> match ip dst 127.0.0.10/32 flowid 1:10 \ >> action vlan push id 101 protocol 802.1q \ >> action drop index 104 >> >> # >> sudo tc -s filter ls dev lo parent ffff: protocol ip >> >> #this will now delete action gact index 104(drop) from display >> sudo tc -s actions get action drop index 104 >> >> sudo tc -s filter ls dev lo parent ffff: protocol ip >> >> #But you can still see it if you do this: >> sudo tc -s actions get action drop index 104 It seems that culprit in this case is tc_action->order field. It is used as nla attrtype when dumping actions. Initially it is set according to ordering of actions of filter that creates them. However, it is overwritten in tca_action_gd() each time action is dumped through action API (according to action position in tb array) and when new filter is attached to shared action (according to action order on the filter). With this we have another way to reproduce the bug: sudo tc qdisc add dev lo ingress #shared action is the first action (order 1) sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ match ip dst 127.0.0.8/32 flowid 1:10 \ action drop index 104 \ action vlan push id 100 protocol 802.1q #shared action is the the second action (order 2) sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ match ip dst 127.0.0.10/32 flowid 1:10 \ action vlan push id 101 protocol 802.1q \ action drop index 104 # Now action is only visible on one filter sudo tc -s filter ls dev lo parent ffff: protocol ip The usage of tc_action->order is inherently incorrect for shared actions and I don't really understand the reason for using it in first place. I'm sending RFC patch that fixes the issue by just using action position in tb array for attrtype instead of order field, and it seems to solve both issues for me. Please check it out to verify that I'm not breaking something. Also, please advise on "fixes" tag since this problem doesn't seem to be directly caused by my act API refactoring.
On 2019-05-22 11:08 a.m., Vlad Buslov wrote: > > On Tue 21 May 2019 at 16:23, Vlad Buslov <vladbu@mellanox.com> wrote: > > It seems that culprit in this case is tc_action->order field. It is used > as nla attrtype when dumping actions. Initially it is set according to > ordering of actions of filter that creates them. However, it is > overwritten in tca_action_gd() each time action is dumped through action > API (according to action position in tb array) and when new filter is > attached to shared action (according to action order on the filter). > With this we have another way to reproduce the bug: > > sudo tc qdisc add dev lo ingress > > #shared action is the first action (order 1) > sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ > match ip dst 127.0.0.8/32 flowid 1:10 \ > action drop index 104 \ > action vlan push id 100 protocol 802.1q > > #shared action is the the second action (order 2) > sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ > match ip dst 127.0.0.10/32 flowid 1:10 \ > action vlan push id 101 protocol 802.1q \ > action drop index 104 > > # Now action is only visible on one filter > sudo tc -s filter ls dev lo parent ffff: protocol ip > Ok, thanks for chasing this. A test case i had in mind is to maybe have 3 actions. Add the drop in the middle for one and at the begging for another and see if they are visible with the patch. If you dont have time I can test maybe AM tommorow. > The usage of tc_action->order is inherently incorrect for shared actions > and I don't really understand the reason for using it in first place. > I'm sending RFC patch that fixes the issue by just using action position > in tb array for attrtype instead of order field, and it seems to solve > both issues for me. Please check it out to verify that I'm not breaking > something. Also, please advise on "fixes" tag since this problem doesn't > seem to be directly caused by my act API refactoring. > I dont know when this broke then. Seems to be working correctly on the machine i am right now (4.4) but broken on 4.19. So somewhere in between things broke. I dont have other kernels to compare at the moment. cheers, jamal
On Wed 22 May 2019 at 20:24, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 2019-05-22 11:08 a.m., Vlad Buslov wrote: >> >> On Tue 21 May 2019 at 16:23, Vlad Buslov <vladbu@mellanox.com> wrote: > >> >> It seems that culprit in this case is tc_action->order field. It is used >> as nla attrtype when dumping actions. Initially it is set according to >> ordering of actions of filter that creates them. However, it is >> overwritten in tca_action_gd() each time action is dumped through action >> API (according to action position in tb array) and when new filter is >> attached to shared action (according to action order on the filter). >> With this we have another way to reproduce the bug: >> >> sudo tc qdisc add dev lo ingress >> >> #shared action is the first action (order 1) >> sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ >> match ip dst 127.0.0.8/32 flowid 1:10 \ >> action drop index 104 \ >> action vlan push id 100 protocol 802.1q >> >> #shared action is the the second action (order 2) >> sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ >> match ip dst 127.0.0.10/32 flowid 1:10 \ >> action vlan push id 101 protocol 802.1q \ >> action drop index 104 >> >> # Now action is only visible on one filter >> sudo tc -s filter ls dev lo parent ffff: protocol ip >> > > Ok, thanks for chasing this. A test case i had in mind is to > maybe have 3 actions. Add the drop in the middle for one > and at the begging for another and see if they are visible > with the patch. > If you dont have time I can test maybe AM tommorow. Works with my patch: ~$ sudo tc qdisc del dev lo ingress ~$ sudo tc qdisc add dev lo ingress ~$ sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ > match ip dst 127.0.0.8/32 flowid 1:10 \ > action drop index 104 \ > mirred egress redirect dev ens1f0 \ > action vlan push id 100 protocol 802.1q ~$ sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \ > match ip dst 127.0.0.10/32 flowid 1:10 \ > action vlan push id 101 protocol 802.1q \ > action drop index 104 \ > mirred egress redirect dev ens1f0 ~$ sudo tc -s filter ls dev lo parent ffff: protocol ip filter pref 8 u32 chain 0 filter pref 8 u32 chain 0 fh 800: ht divisor 1 filter pref 8 u32 chain 0 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10 not_in_hw (rule hit 0 success 0) match 7f000008/ffffffff at 16 (success 0 ) action order 1: gact action drop random type none pass val 0 index 104 ref 2 bind 2 installed 8 sec used 8 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 2: mirred (Egress Redirect to device ens1f0) stolen index 1 ref 1 bind 1 installed 8 sec used 8 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 3: vlan push id 100 protocol 802.1Q priority 0 pipe index 1 ref 1 bind 1 installed 8 sec used 8 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 filter pref 8 u32 chain 0 fh 800::801 order 2049 key ht 800 bkt 0 flowid 1:10 not_in_hw (rule hit 0 success 0) match 7f00000a/ffffffff at 16 (success 0 ) action order 1: vlan push id 101 protocol 802.1Q priority 0 pipe index 2 ref 1 bind 1 installed 4 sec used 4 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 2: gact action drop random type none pass val 0 index 104 ref 2 bind 2 installed 8 sec used 8 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 3: mirred (Egress Redirect to device ens1f0) stolen index 2 ref 1 bind 1 installed 4 sec used 4 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0
On 2019-05-22 1:49 p.m., Vlad Buslov wrote: > > On Wed 22 May 2019 at 20:24, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> Ok, thanks for chasing this. A test case i had in mind is to >> maybe have 3 actions. Add the drop in the middle for one >> and at the begging for another and see if they are visible >> with the patch. >> If you dont have time I can test maybe AM tommorow. > > Works with my patch: > Ok, thanks Vlad. Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal
On 2019-05-22 2:23 p.m., Jamal Hadi Salim wrote: > On 2019-05-22 1:49 p.m., Vlad Buslov wrote: >> >> On Wed 22 May 2019 at 20:24, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > >>> Ok, thanks for chasing this. A test case i had in mind is to >>> maybe have 3 actions. Add the drop in the middle for one >>> and at the begging for another and see if they are visible >>> with the patch. >>> If you dont have time I can test maybe AM tommorow. >> >> Works with my patch: >> > > Ok, thanks Vlad. > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> > > cheers, > jamal Actually this is net/stable material. Can you resubmit to net and add my ACK? cheers, jamal