Two more comments to this patch…
On 7 Sep 2021, at 10:01, 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 <[email protected]> >> Reviewed-by: Eli Britstein <[email protected]> > > 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? In addition we should also make sure att->userdata_len is at least equal to sizeof *cookie. >> + 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; Here you should use the attr value? &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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
