[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