Re: [ovs-dev] [PATCH v22 6/8] dpif-netlink: Add netdev offload recv in normal recv upcalls
On 2/23/2023 9:43 PM, Ilya Maximets wrote: 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 Reviewed-by: Roi Dayan --- 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(>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(>upcall_lock); +if (handler_id) { +error = dpif_netlink_recv__(dpif_, handler_id, upcall, buf); +fat_rwlock_unlock(>upcall_lock); +return error; +} + +handler = >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(>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(>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(>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
Re: [ovs-dev] [PATCH v22 6/8] dpif-netlink: Add netdev offload recv in normal recv upcalls
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 > Reviewed-by: Roi Dayan > --- > 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(>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(>upcall_lock); > +if (handler_id) { > +error = dpif_netlink_recv__(dpif_, handler_id, upcall, buf); > +fat_rwlock_unlock(>upcall_lock); > +return error; > +} > + > +handler = >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(>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(>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(>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
[ovs-dev] [PATCH v22 6/8] dpif-netlink: Add netdev offload recv in normal recv upcalls
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 Reviewed-by: Roi Dayan --- 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(>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(>upcall_lock); +if (handler_id) { +error = dpif_netlink_recv__(dpif_, handler_id, upcall, buf); +fat_rwlock_unlock(>upcall_lock); +return error; +} + +handler = >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(>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(>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(>tunnel, dupcall->in_tun, sizeof flow->tunnel); +} +flow->in_port.odp_port = +netdev_ifindex_to_odp_port(dupcall->iifindex); } if (dupcall->mru) { -- 2.26.3 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev