[ovs-dev] [PATCH v7 11/11] netdev-offload-tc: Add offload support for sFlow

Chris Mi cmi at nvidia.com
Fri Dec 4 05:34:53 UTC 2020


On 12/2/2020 11:08 PM, Eelco Chaudron wrote:
>
>
> On 30 Nov 2020, at 5:48, Chris Mi wrote:
>
>> Create a unique group ID to map the sFlow info when offloading sFlow
>> action to TC. When showing the offloaded datapath flows, translate the
>> group ID from TC sample action to sFlow info using the mapping.
>>
>> Signed-off-by: Chris Mi <cmi at nvidia.com>
>> Reviewed-by: Eli Britstein <elibr at nvidia.com>
>>
>
> <SNIP>
>
> This is not a full review, I was just trying to understand how you 
> implemented the conditional action set as I’m working on something 
> similar for dec_ttl.
>
> However, I noticed that you ignore this condition entirely, which I 
> think should be solved one way or the other in this patchset. See my 
> comments below.
>
>> @@ -2061,17 +2147,49 @@ netdev_tc_flow_put(struct netdev *netdev, 
>> struct match *match,
>>              struct dpif_sflow_attr sflow_attr;
>>
>>              memset(&sflow_attr, 0, sizeof sflow_attr);
>> -            gid_alloc_ctx(&sflow_attr);
>> +            if (flower.tunnel) {
>> +                sflow_attr.tunnel = CONST_CAST(struct flow_tnl *, tnl);
>> +            }
>> +            err = parse_sample_action(nla, &sflow_attr, action);
>
> The main problem here (in the above call) you only look for 
> OVS_ACTION_ATTR_USERSPACE, if it exists you are ok and handle ONLY 
> that action.
> If there are more actions like “userspace(),1,2”, they output to ports 
> 1 and 2 are ignored. Guess for your implementation you should only 
> allow TC offload if the action set consists of a single userspace() 
> action.
If sample rate is not 1, the sample action looks like this:
actions:sample(sample=50.0%,actions(userspace(pid=2619320998,sFlow(vid=0,pcp=0,output=17),actions))),enp4s0f0_1

If sample rate is 1, the sample action looks like this:
actions:userspace(pid=2619320998,sFlow(vid=0,pcp=0,output=17),actions),enp4s0f0_1

In your case, userspace(),1,2, if there is a sFlow cookie inside 
userspace action, we can offload. Otherwise, we don't offload.

In the code, we deal with above 2 situations like this:
...
         } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_SAMPLE) {
...
         } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
...
>
>> +            if (err) {
>> +                goto out;
>> +            }
>> +            group_id = gid_alloc_ctx(&sflow_attr);
>> +            action->sample.action_group_id = group_id;
>> +            flower.action_count++;
>
> I think these last three commands should be done as part of 
> parse_sample_action() as all other "if" conditions do either all in 
> the branch or in the called function.
Please see my above reply. There are two situations. I have comment in 
the code:

2160         } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
2161             struct dpif_sflow_attr sflow_attr;
2162
2163             /* If there is a sFlow cookie inside of a userspace 
attribute,
2164              * but no sample attribute, that means sampling rate is 1.
2165              */
>
>
>> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
>
> The OVS_ACTION_ATTR_USERSPACE should not be handled here as a separate 
> action, at least it should not end up as a TC_ACT_SAMPLE. What if 
> someone else is having an OVS_ACTION_ATTR_USERPSACE in its chain, now 
> it will be translated into a SAMPLE action.
>
> The parse_sample_action() function called above should loop trough the 
> OVS_SAMPLE_ATTR_ACTIONS and find the OVS_ACTION_ATTR_USERSPACE, which 
> it looks like it does. So this “if (nl_attr_type(nla) == 
> OVS_ACTION_ATTR_USERSPACE)” part should not be needed at all.
Please see above reply.
>
>> +            struct dpif_sflow_attr sflow_attr;
>> +
>> +            /* If there is a sFlow cookie inside of a userspace 
>> attribute,
>> +             * but no sample attribute, that means sampling rate is 1.
>> +             */
>> +            memset(&sflow_attr, 0, sizeof sflow_attr);
>> +            if (flower.tunnel) {
>> +                sflow_attr.tunnel = CONST_CAST(struct flow_tnl *, tnl);
>> +            }
>> +            err = parse_userspace_userdata(nla, &sflow_attr);
>> +            if (err) {
>> +                goto out;
>> +            }
>> +            sflow_attr.sflow = nla;
>> +            sflow_attr.sflow_len = nla->nla_len;
>> +            group_id = gid_alloc_ctx(&sflow_attr);
>> +            action->type = TC_ACT_SAMPLE;
>> +            action->sample.action_group_id = group_id;
>> +            action->sample.action_rate = 1;
>> +            flower.action_count++;
>>          } else {
>>              VLOG_DBG_RL(&rl, "unsupported put action type: %d",
>>                          nl_attr_type(nla));
>> -            return EOPNOTSUPP;
>> +            err = EOPNOTSUPP;
>> +            goto out;
>>          }
>>      }
>>
>
> <SNIP>
>
> One other general command, how would we guarantee that the group ID 
> value used by tc-sample is unique and not being used by any other 
> application than OVS? 
Please see the comment:

  239 /* Allocate a unique group id for the given set of flow metadata.
  240  * The ID space is 2^^32, so there should never be a situation in 
which all
  241  * the IDs are used up.  We loop until we find a free one. */
  242 static struct gid_node *
  243 gid_alloc__(const struct dpif_sflow_attr *sflow, uint32_t hash)

This id is similar to recirc id.
> Do we assume tc-sample is elusively assigned to OVS?
Yes, I think so.
> This might not be the case in customer environments, so we might need 
> to be able to disable offloading this action.
If we can offload, why not disable it. If we can't offload, tc will 
return EOPNOTSUPP and fail back to ovs datapath.
So I don't think we need to be able to disable it.
>
> One final question, do you know if this will be supported in your NICs 
> as hardware offload?
Sure. We submitted the first version of the OVS patch set in September. 
We support offload before that.
I believe the driver change will be in linux net-next branch soon.

Thanks,
Chris
>
> Cheers,
>
> Eelco
>



More information about the dev mailing list