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

[Chris Mi]

Hi Eelco,

Thanks for your review.

On 12/14/2020 7:46 PM, Eelco Chaudron wrote:
> Hi Chris,
>
> Noticed your v8, will try to fully review it before the year is over ;)
>
> //Eelco
>
> On 14 Dec 2020, at 10:33, Chris Mi wrote:
>
> <SNIP>
>
>>> diff --git a/lib/dpif.h b/lib/dpif.h
>>> index ed4257210..150162034 100644
>>> --- a/lib/dpif.h
>>> +++ b/lib/dpif.h
>>> @@ -878,6 +878,7 @@ void dpif_register_upcall_cb(struct dpif *, 
>>> upcall_callback *, void *aux);
>>>  struct dpif_sflow_attr {
>>>      const struct nlattr *sflow; /* sFlow action */
>>>      size_t sflow_len;           /* Length of 'sflow' in bytes. */
>>> +    // Why do we need to store sflow_len, it's part of the nlattr 
>>> data?
>> We need to copy and compare the dpif_sflow_attr. So sflow_len is used 
>> for that.
>
> But you could just use sflow->nla_len?
Done.
>
>>>      void *userdata;             /* struct user_action_cookie */
>>>      size_t userdata_len;        /* struct user_action_cookie length */
>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>> index 75744aa3c..84991ff6a 100644
>>>
>>>  static int
>>> -parse_action_userspace(const struct nlattr *actions,
>>> -                       struct dpif_sflow_attr *sflow_attr)
>>> +parse_sample_actions(const struct nlattr *actions,
>>> +                     struct dpif_sflow_attr *sflow_attr)
>>>  {
>>>      const struct nlattr *nla;
>>>      unsigned int left;
>>> +    int err = EINVAL;
>>>
>>>      NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
>>>          if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
>>> -            return parse_userspace_userdata(nla, sflow_attr);
>>> +            err = parse_userspace_userdata(nla, sflow_attr);
>>> +        } else {
>>> +            /* We can't offload other nested actions */
>>> +            VLOG_DBG_RL(&error_rl,
>>> +                        "%s: other than OVS_ACTION_ATTR_USERSPACE 
>>> attribute",
>>> +                        __func__);
>>> +            return EINVAL;
>>>          }
>>>      }
>> I think we should not log in this function. We may be overwhelmed for 
>> various usespace actions.
>
> It’s a debug message, and it might be useful to figure out why offload 
> is failing, especially as this is a corner case.
Done.
>
>>> -    VLOG_ERR_RL(&error_rl, "%s: no OVS_ACTION_ATTR_USERSPACE 
>>> attribute",
>>> -                __func__);
>>> -    return EINVAL;
>>> +    if (err) {
>>> +        //THIS IS NOT AN ERROR CASE, FROM A DATAPATH IT'S VALID TO 
>>> DO ANY ACTION
>> But sample action expects a userspace attribute.
>
> This is the typical use case, but I think you can add one without it 
> and just redirect the packet to a different port. I don’t have a setup 
> right now, but I will try some examples when I’ll do the v8 review.
>
>>> + VLOG_DBG_RL(&error_rl, "%s: no OVS_ACTION_ATTR_USERSPACE attribute",
>>> +                    __func__);
>>> +    }
>>> +    return err;
>>>  }
>>>
>
> <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.
>>>
>>> Ack all three above, but I was referring to someone that uses sflow 
>>> kmod on his management interface already, and now all of a sudden 
>>> OVS will use it too. Which could cause a problem as the u32 space is 
>>> started between all user space application. So I do think we need an 
>>> option to disable it in OVS.
>> Sorry, what do you mean by sflow kmod?
>
> I mean the psample kmod, tc-sample feature.
OK, I see what you mean. But we can disable offload via:
# ovs-vsctl set Open_vSwitch . other_config:hw-offload="false";

sFlow is not per flow. If user creates OVS sflow, I think almost all 
flows will have an additional sFlow action.
I don't think we need to disable sFlow action only. That means we 
disable offload for all flows.
>
>>>>>
>>>>> 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.
>>>
>>> Cool, looking forward to seeing the change…
>>>
>>>
>>> If you do send out a new revision of this patchset, I’ll try to make 
>>> some time and do a full review and some testing.
>>>
>> V8 will be sent for the full review.
I'll send v9 for above two small changes.

Thanks,
Chris