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

[Eelco Chaudron]

On 15 Jul 2021, at 8:01, Chris Mi wrote:

> Process sFlow offload packet in handler thread if handler id is 0.
>
> Signed-off-by: Chris Mi <cmi at nvidia.com>
> Reviewed-by: Eli Britstein <elibr at nvidia.com>

Thanks for making these changes, as this implementation looks way 
cleaner than it was before!

> ---
>  ofproto/ofproto-dpif-upcall.c | 57 
> +++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c 
> b/ofproto/ofproto-dpif-upcall.c
> index ccf97266c..7e934614d 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"
> @@ -742,6 +743,51 @@ 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;
> +    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: dpif_sflow_attr is NULL", __func__);

As these are more user log messages, not debug, I would change it to 
something like:

         VLOG_WARN_RL(&rl, "sFlow upcall is missing its attribute");

> +        return;
> +    }
> +
> +    cookie = attr->userdata;

Just to be safe should we also check if cookie is not NULL + log 
message?

> +    ofproto = ofproto_dpif_lookup_by_uuid(&cookie->ofproto_uuid);
> +    if (!ofproto) {
> +        VLOG_WARN_RL(&rl, "%s: could not find ofproto", __func__);

Maybe here we could also print the UUID, so it makes it possible to 
debug for which tc rule the upcall was.
         VLOG_WARN_RL(&rl, "sFlow upcall can't find ofproto dpif for 
UUID "UUID_FMT", ...)

> +        return;
> +    }
> +
> +    dpif_sflow = ofproto->sflow;
> +    if (!sflow) {

Guess this needs to be dpif_sflow

> +        VLOG_WARN_RL(&rl, "%s: could not find dpif_sflow", __func__);

Guess there is no find here, so it might be the same as the first LOG 
above:
         VLOG_WARN_RL(&rl, "sFlow upcall is missing dpif information");


> +        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 = &sflow->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. */
> @@ -756,8 +802,19 @@ udpif_upcall_handler(void *arg)
>              poll_immediate_wake();
>          } else {
>              dpif_recv_wait(udpif->dpif, handler->handler_id);
> +            dpif_offload_sflow_recv_wait(udpif->dpif);

Guess this only needs to be called from handler 0, same as below.

>              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;
> +
> +            err = dpif_offload_sflow_recv(udpif->dpif, &sflow);
> +            if (!err) {
> +                process_offload_sflow(udpif, &sflow);
> +            }
> +        }
>          poll_block();
>      }
>
> -- 
> 2.21.0