On 2/23/23 12:27, Chris Mi wrote: > In thread handler 0, add netdev offload recv in normal recv upcalls. > To avoid starvation, introduce a flag to alternate the order of > receiving normal upcalls and offload upcalls based on that flag. > > Add similar change for recv_wait. > > Signed-off-by: Chris Mi <[email protected]> > Reviewed-by: Roi Dayan <[email protected]> > --- > lib/dpif-netlink.c | 46 ++++++++++++++++++++++++++++++++--- > ofproto/ofproto-dpif-upcall.c | 23 +++++++++++++++--- > 2 files changed, 62 insertions(+), 7 deletions(-) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index 586fb8893..9f67db1be 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -201,6 +201,12 @@ struct dpif_handler { > struct nl_sock *sock; /* Each handler thread holds one netlink > socket. */ > > + /* Thread handler 0 deals with both netdev offload recv and normal > + * recv upcalls. To avoid starvation, introduce a flag to alternate > + * the order. > + */ > + bool recv_offload_first; > + > #ifdef _WIN32 > /* Pool of sockets. */ > struct dpif_windows_vport_sock *vport_sock_pool; > @@ -3130,13 +3136,12 @@ dpif_netlink_recv_vport_dispatch(struct dpif_netlink > *dpif, > #endif > > static int > -dpif_netlink_recv(struct dpif *dpif_, uint32_t handler_id, > - struct dpif_upcall *upcall, struct ofpbuf *buf) > +dpif_netlink_recv__(struct dpif *dpif_, uint32_t handler_id, > + struct dpif_upcall *upcall, struct ofpbuf *buf) > { > struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); > int error; > > - fat_rwlock_rdlock(&dpif->upcall_lock); > #ifdef _WIN32 > error = dpif_netlink_recv_windows(dpif, handler_id, upcall, buf); > #else > @@ -3147,6 +3152,38 @@ dpif_netlink_recv(struct dpif *dpif_, uint32_t > handler_id, > handler_id, upcall, buf); > } > #endif > + > + return error; > +} > + > +static int > +dpif_netlink_recv(struct dpif *dpif_, uint32_t handler_id, > + struct dpif_upcall *upcall, struct ofpbuf *buf) > +{ > + struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); > + struct dpif_handler *handler; > + int error; > + > + fat_rwlock_rdlock(&dpif->upcall_lock); > + if (handler_id) { > + error = dpif_netlink_recv__(dpif_, handler_id, upcall, buf); > + fat_rwlock_unlock(&dpif->upcall_lock); > + return error; > + } > + > + handler = &dpif->handlers[handler_id]; > + if (handler->recv_offload_first) { > + error = netdev_offload_recv(upcall, buf); > + if (error == EAGAIN) { > + error = dpif_netlink_recv__(dpif_, handler_id, upcall, buf); > + } > + } else { > + error = dpif_netlink_recv__(dpif_, handler_id, upcall, buf); > + if (error == EAGAIN) { > + error = netdev_offload_recv(upcall, buf); > + } > + } > + handler->recv_offload_first = !handler->recv_offload_first; > fat_rwlock_unlock(&dpif->upcall_lock); > > return error; > @@ -3211,6 +3248,9 @@ dpif_netlink_recv_wait(struct dpif *dpif_, uint32_t > handler_id) > } else { > dpif_netlink_recv_wait_vport_dispatch(dpif, handler_id); > } > + if (handler_id == 0) { > + netdev_offload_recv_wait(); > + } > #endif > fat_rwlock_unlock(&dpif->upcall_lock); > } > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index fc94078cb..273b576bd 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -840,10 +840,25 @@ recv_upcalls(struct handler *handler) > break; > } > > - upcall->fitness = odp_flow_key_to_flow(dupcall->key, > dupcall->key_len, > - flow, NULL); > - if (upcall->fitness == ODP_FIT_ERROR) { > - goto free_dupcall; > + /* If it is normal upcalls, datapath will provide key and key_len > + * to construct flow. But for netdev offload upcalls, key and > + * key_len are not available. Construct partial flow using available > + * info.> + */ > + if (dupcall->key && dupcall->key_len) { > + upcall->fitness = odp_flow_key_to_flow(dupcall->key, > + dupcall->key_len, > + flow, NULL); > + if (upcall->fitness == ODP_FIT_ERROR) { > + goto free_dupcall; > + } > + } else { > + memset(flow, 0, sizeof *flow); > + if (dupcall->in_tun) { > + memcpy(&flow->tunnel, dupcall->in_tun, sizeof flow->tunnel); > + } > + flow->in_port.odp_port = > + netdev_ifindex_to_odp_port(dupcall->iifindex);
I didn't read the whole set, but this doesn't look right. In particular, calling netdev_ifindex_to_odp_port() from the generic upcall processing code. I agree though that it doesn't make a lot of sense to have a key/key_len for an sFlow upcall. A better approach would be to add an actual struct flow to the dupcall structure. You're already adding a tunnel metadata there, we could just add a full flow instead and make offload layer to prepare it. In that case recv_upcalls() may check if it has key to parse or it already has a parsed flow structure. No HWOL-specific calls like netdev_ifindex_to_odp_port() will be needed in the generic code then. And it will be netdev-offload layer's responsibility to properly fill in a flow structure. P.S. many patches in this patch set are failing thread-safety analysis. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
