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

[Eelco Chaudron]

On 22 Jan 2021, at 6:15, Chris Mi wrote:

On 1/13/2021 4:27 AM, Eelco Chaudron wrote:

On 15 Dec 2020, at 4:38, Chris Mi wrote:

<SNIP>

    struct nlattr *nla;
    struct tcf_id id;
    uint32_t chain;
    @@ -1972,7 +2098,8 @@ netdev_tc_flow_put(struct netdev *netdev,
    struct match *match,
    NL_ATTR_FOR_EACH(nla, left, actions, actions_len) {
    if (flower.action_count >= TCA_ACT_MAX_NUM) {
    VLOG_DBG_RL(&rl, "Can only support %d actions", TCA_ACT_MAX_NUM);
    - return EOPNOTSUPP;
    + err = EOPNOTSUPP;
    + goto out;
    }
    action = &flower.actions[flower.action_count];
    if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
    @@ -1982,7 +2109,8 @@ netdev_tc_flow_put(struct netdev *netdev,
    struct match *match,
    if (!outdev) {
    VLOG_DBG_RL(&rl, "Can't find netdev for output port %d", port);
    - return ENODEV;
    + err = ENODEV;
    + goto out;
    }
    action->out.ifindex_out = netdev_get_ifindex(outdev);
    action->out.ingress = is_internal_port(netdev_get_type(outdev));
    @@ -2020,7 +2148,7 @@ netdev_tc_flow_put(struct netdev *netdev,
    struct match *match,
    err = parse_put_flow_set_action(&flower, action, set, set_len);
    if (err) {
    - return err;
    + goto out;
    }
    if (action->type == TC_ACT_ENCAP) {
    action->encap.tp_dst = info->tp_dst_port;
    @@ -2033,7 +2161,7 @@ netdev_tc_flow_put(struct netdev *netdev,
    struct match *match,
    err = parse_put_flow_set_masked_action(&flower, action, set,
    set_len, true);
    if (err) {
    - return err;
    + goto out;
    }
    } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_CT) {
    const struct nlattr *ct = nl_attr_get(nla);
    @@ -2041,7 +2169,7 @@ netdev_tc_flow_put(struct netdev *netdev,
    struct match *match,
    err = parse_put_flow_ct_action(&flower, action, ct, ct_len);
    if (err) {
    - return err;
    + goto out;
    }
    } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_CT_CLEAR) {
    action->type = TC_ACT_CT;
    @@ -2057,20 +2185,27 @@ netdev_tc_flow_put(struct netdev *netdev,
    struct match *match,
    action->chain = 0; /* 0 is reserved and not used by recirc. */
    flower.action_count++;
    } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_SAMPLE) {
    - struct dpif_sflow_attr sflow_attr;
    -
    - memset(&sflow_attr, 0, sizeof sflow_attr);
    - gid_alloc_ctx(&sflow_attr);
    + err = parse_sample_action(&flower, action, nla, tnl, 
&group_id);
    + if (err) {
    + goto out;
    + }

For this case and the case below we should probably error out if the 
group_id is already used?

I don't think the used group_id can be allocated.

From the command line you can probably add a dpath rule with two 
sample actions.

Yes, from tc command line, we can add two sample actions. But I 
believe ovs can't do it.
Maybe tc should prevent it if user specifies two sample actions. It 
doesn't help if we check it here.

I think this is the perfect spot to add the check, it’s where it’s 
done for other TC related actions.
So you could just add:

        if (group_id) {
         VLOG_ERR_RL(&error_rl, “Only a single TC_SAMPLE action per 
flow is supported” );
         err = EOPNOTSUPP;
         goto out;
    }   

Or you add it to both parse_sample_action() and 
parse_userspace_action().

    + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
    + err = parse_userspace_action(&flower, action, nla, tnl, 
&group_id);
    + if (err) {
    + goto out;
    + }
    } else {
    VLOG_DBG_RL(&rl, "unsupported put action type: %d",
    nl_attr_type(nla));
    - return EOPNOTSUPP;
    + err = EOPNOTSUPP;
    + goto out;
    }
    }

<SNIP>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev