[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