[ovs-dev] [PATCH v14 6/7] ofproto: Introduce API to process sFlow offload packet
Chris Mi
cmi at nvidia.com
Wed Sep 15 10:17:08 UTC 2021
On 9/7/2021 4:01 PM, Eelco Chaudron wrote:
>
> 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");|
|Done.|
> ||
>
> + 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", ...)
>
Done.
>
> + return;
> + }
> +
> + dpif_sflow = ofproto->sflow;
> + if (!sflow) {
>
> Guess this needs to be dpif_sflow
>
Indeed, done.
>
> + 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");
>
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 = &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.
>
I changed it like this before. But it didn't work. We can't receive
sampled packets.
I tested this again. Now it works. Not sure what changed :(
Done.
>
> 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
>
More information about the dev
mailing list