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 <[email protected]> > Reviewed-by: Eli Britstein <[email protected]> > --- > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
