[ovs-dev,v15,6/7] ofproto: Introduce API to process sFlow offload packet

Process sFlow offload packet in handler thread if handler id is 0.

Signed-off-by: Chris Mi <cmi@nvidia.com>
Reviewed-by: Eli Britstein <elibr@nvidia.com>
---
 ofproto/ofproto-dpif-upcall.c | 63 +++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

See comments below.

On 15 Sep 2021, at 14:43, Chris Mi wrote:

> Process sFlow offload packet in handler thread if handler id is 0.
>
> Signed-off-by: Chris Mi <cmi@nvidia.com>
> Reviewed-by: Eli Britstein <elibr@nvidia.com>
> ---
>  ofproto/ofproto-dpif-upcall.c | 63 +++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 1c9c720f0..4a36a45bb 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -22,6 +22,7 @@
>  #include "connmgr.h"
>  #include "coverage.h"
>  #include "cmap.h"
> +#include "lib/dpif-offload-provider.h"
>  #include "lib/dpif-provider.h"
>  #include "dpif.h"
>  #include "openvswitch/dynamic-string.h"
> @@ -779,6 +780,57 @@ udpif_get_n_flows(struct udpif *udpif)
>      return flow_count;
>  }
>
> +static void
> +process_offload_sflow(struct udpif *udpif, struct dpif_offload_sflow *sflow)
> +{
> +    const struct dpif_sflow_attr *attr = sflow->attr;
> +    const struct user_action_cookie *cookie;
> +    struct dpif_sflow *dpif_sflow;
> +    struct ofproto_dpif *ofproto;
> +    struct upcall upcall;
> +    uint32_t iifindex;
> +    struct flow flow;
> +
> +    if (!attr) {
> +        VLOG_WARN_RL(&rl, "%s: sFlow upcall is missing its attribute",
> +                     __func__);

Here we should remove the function in the log message. A developer can easily find it, and it might confuse the end-user.

> +        return;
> +    }
> +
> +    cookie = nl_attr_get(attr->userdata);

Here we need to also check that the length of the attr is at least sizeof(struct user_action_cookie)

> +    if (!cookie) {
> +        VLOG_WARN_RL(&rl, "%s: user action cookie is missing", __func__);

Remove function name, and make it sflow specific;

“sFlow user action cookie is missing”

> +        return;
> +    }
> +    ofproto = ofproto_dpif_lookup_by_uuid(&cookie->ofproto_uuid);
> +    if (!ofproto) {
> +        VLOG_WARN_RL(&rl, "%s: sFlow upcall can't find ofproto dpif for UUID "
> +                     UUID_FMT, __func__, UUID_ARGS(&cookie->ofproto_uuid));

Please remove function name.

> +        return;
> +    }
> +    dpif_sflow = ofproto->sflow;
> +    if (!dpif_sflow) {
> +        VLOG_WARN_RL(&rl, "%s: sFlow upcall is missing dpif information",
> +                     __func__);

Please remove function name.

> +        return;
> +    }
> +
> +    memset(&flow, 0, sizeof flow);
> +    if (attr->tunnel) {
> +        memcpy(&flow.tunnel, attr->tunnel, sizeof flow.tunnel);
> +    }
> +    iifindex = sflow->iifindex;
> +    flow.in_port.odp_port = netdev_ifindex_to_odp_port(iifindex);
> +    memset(&upcall, 0, sizeof upcall);
> +    upcall.flow = &flow;
> +    upcall.cookie = *cookie;
> +    upcall.packet = &sflow->packet;
> +    upcall.sflow = dpif_sflow;
> +    upcall.ufid = &attr->ufid;
> +    upcall.type = SFLOW_UPCALL;
> +    process_upcall(udpif, &upcall, NULL, NULL);
> +}
> +
>  /* The upcall handler thread tries to read a batch of UPCALL_MAX_BATCH
>   * upcalls from dpif, processes the batch and installs corresponding flows
>   * in dpif. */
> @@ -795,6 +847,17 @@ udpif_upcall_handler(void *arg)
>              dpif_recv_wait(udpif->dpif, handler->handler_id);
>              latch_wait(&udpif->exit_latch);
>          }
> +        /* Only handler id 0 thread process sFlow offload packet. */
> +        if (handler->handler_id == 0) {
> +            struct dpif_offload_sflow sflow;
> +            int err;
> +
> +            dpif_offload_sflow_recv_wait(udpif->dpif);
> +            err = dpif_offload_sflow_recv(udpif->dpif, &sflow);
> +            if (!err) {
> +                process_offload_sflow(udpif, &sflow);
> +            }

Here we only read and process a single sflow upcall for each poll block. Should we not do the same as the upcall handling? It's reading max 64 entries before continuation?

> +        }
>          poll_block();
>      }
>
> -- 
> 2.27.0

On 10/1/2021 8:24 PM, Eelco Chaudron wrote:
> See comments below.
>
> On 15 Sep 2021, at 14:43, Chris Mi wrote:
>
>> Process sFlow offload packet in handler thread if handler id is 0.
>>
>> Signed-off-by: Chris Mi <cmi@nvidia.com>
>> Reviewed-by: Eli Britstein <elibr@nvidia.com>
>> ---
>>   ofproto/ofproto-dpif-upcall.c | 63 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 63 insertions(+)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 1c9c720f0..4a36a45bb 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -22,6 +22,7 @@
>>   #include "connmgr.h"
>>   #include "coverage.h"
>>   #include "cmap.h"
>> +#include "lib/dpif-offload-provider.h"
>>   #include "lib/dpif-provider.h"
>>   #include "dpif.h"
>>   #include "openvswitch/dynamic-string.h"
>> @@ -779,6 +780,57 @@ udpif_get_n_flows(struct udpif *udpif)
>>       return flow_count;
>>   }
>>
>> +static void
>> +process_offload_sflow(struct udpif *udpif, struct dpif_offload_sflow *sflow)
>> +{
>> +    const struct dpif_sflow_attr *attr = sflow->attr;
>> +    const struct user_action_cookie *cookie;
>> +    struct dpif_sflow *dpif_sflow;
>> +    struct ofproto_dpif *ofproto;
>> +    struct upcall upcall;
>> +    uint32_t iifindex;
>> +    struct flow flow;
>> +
>> +    if (!attr) {
>> +        VLOG_WARN_RL(&rl, "%s: sFlow upcall is missing its attribute",
>> +                     __func__);
> Here we should remove the function in the log message. A developer can easily find it, and it might confuse the end-user.
>
>> +        return;
>> +    }
>> +
>> +    cookie = nl_attr_get(attr->userdata);
> Here we need to also check that the length of the attr is at least sizeof(struct user_action_cookie)
Done.
>
>> +    if (!cookie) {
>> +        VLOG_WARN_RL(&rl, "%s: user action cookie is missing", __func__);
> Remove function name, and make it sflow specific;
>
> “sFlow user action cookie is missing”
Done.
>
>> +        return;
>> +    }
>> +    ofproto = ofproto_dpif_lookup_by_uuid(&cookie->ofproto_uuid);
>> +    if (!ofproto) {
>> +        VLOG_WARN_RL(&rl, "%s: sFlow upcall can't find ofproto dpif for UUID "
>> +                     UUID_FMT, __func__, UUID_ARGS(&cookie->ofproto_uuid));
> Please remove function name.
>
>> +        return;
>> +    }
>> +    dpif_sflow = ofproto->sflow;
>> +    if (!dpif_sflow) {
>> +        VLOG_WARN_RL(&rl, "%s: sFlow upcall is missing dpif information",
>> +                     __func__);
> Please remove function name.
Done.
>
>> +        return;
>> +    }
>> +
>> +    memset(&flow, 0, sizeof flow);
>> +    if (attr->tunnel) {
>> +        memcpy(&flow.tunnel, attr->tunnel, sizeof flow.tunnel);
>> +    }
>> +    iifindex = sflow->iifindex;
>> +    flow.in_port.odp_port = netdev_ifindex_to_odp_port(iifindex);
>> +    memset(&upcall, 0, sizeof upcall);
>> +    upcall.flow = &flow;
>> +    upcall.cookie = *cookie;
>> +    upcall.packet = &sflow->packet;
>> +    upcall.sflow = dpif_sflow;
>> +    upcall.ufid = &attr->ufid;
>> +    upcall.type = SFLOW_UPCALL;
>> +    process_upcall(udpif, &upcall, NULL, NULL);
>> +}
>> +
>>   /* The upcall handler thread tries to read a batch of UPCALL_MAX_BATCH
>>    * upcalls from dpif, processes the batch and installs corresponding flows
>>    * in dpif. */
>> @@ -795,6 +847,17 @@ udpif_upcall_handler(void *arg)
>>               dpif_recv_wait(udpif->dpif, handler->handler_id);
>>               latch_wait(&udpif->exit_latch);
>>           }
>> +        /* Only handler id 0 thread process sFlow offload packet. */
>> +        if (handler->handler_id == 0) {
>> +            struct dpif_offload_sflow sflow;
>> +            int err;
>> +
>> +            dpif_offload_sflow_recv_wait(udpif->dpif);
>> +            err = dpif_offload_sflow_recv(udpif->dpif, &sflow);
>> +            if (!err) {
>> +                process_offload_sflow(udpif, &sflow);
>> +            }
> Here we only read and process a single sflow upcall for each poll block. Should we not do the same as the upcall handling? It's reading max 64 entries before continuation?
Done.
>
>> +        }
>>           poll_block();
>>       }
>>
>> -- 
>> 2.27.0