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
