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 <c...@nvidia.com>
Reviewed-by: Eli Britstein <el...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
