| Message ID | 1608976283-8044-1-git-send-email-wangyunjian@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] upcall: avoid handler block in recv_upcalls | expand |
On 12/26/2020 11:51 AM, wangyunjian wrote: > From: Lvmengfan <lvmengfan@huawei.com> > > When upcall_receive or process_upcall returns error in recv_upcalls, the > n_upcalls count would not increased. If this situation continues, the > loop may failed to exit, then the handler block occurs. Add a n_errors > to count the errors and jump out of the loop, cloud avoid block typo > happening. Fixes: cc377352d164 ("ofproto: Reorganize in preparation for direct dpdk upcalls.") > Signed-off-by: Lvmengfan <lvmengfan@huawei.com> Could you please add a unit-test for this? > --- > ofproto/ofproto-dpif-upcall.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 27924fa..6b32b18 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -776,9 +776,10 @@ recv_upcalls(struct handler *handler) > struct dpif_upcall dupcalls[UPCALL_MAX_BATCH]; > struct upcall upcalls[UPCALL_MAX_BATCH]; > struct flow flows[UPCALL_MAX_BATCH]; > - size_t n_upcalls, i; > + size_t n_upcalls, i, n_errors; > > n_upcalls = 0; > + n_errors = 0; > while (n_upcalls < UPCALL_MAX_BATCH) { Maybe simpler instead of the below free_dupcall hunk, to do like: while (n_upcalls + n_errors < UPCALL_MAX_BATCH) { The downside is that in case of errors we will not fill the entire UPCALL_MAX_BATCH, but simpler. > struct ofpbuf *recv_buf = &recv_bufs[n_upcalls]; > struct dpif_upcall *dupcall = &dupcalls[n_upcalls]; > @@ -810,6 +811,7 @@ recv_upcalls(struct handler *handler) > dupcall->type, dupcall->userdata, flow, mru, > &dupcall->ufid, PMD_ID_NULL); > if (error) { > + n_errors++; > if (error == ENODEV) { > /* Received packet on datapath port for which we couldn't > * associate an ofproto. This can happen if a port is removed > @@ -837,6 +839,7 @@ recv_upcalls(struct handler *handler) > error = process_upcall(udpif, upcall, > &upcall->odp_actions, &upcall->wc); > if (error) { > + n_errors++; > goto cleanup; > } > > @@ -848,6 +851,13 @@ cleanup: > free_dupcall: > dp_packet_uninit(&dupcall->packet); > ofpbuf_uninit(recv_buf); > + if (n_errors >= UPCALL_MAX_BATCH * 2) { Why UPCALL_MAX_BATCH * 2 ? Please add a comment to explain, also the below. > + if (n_upcalls == 0) { > + return 1; > + } else { > + break; > + } > + } > } > > if (n_upcalls) {
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 27924fa..6b32b18 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -776,9 +776,10 @@ recv_upcalls(struct handler *handler) struct dpif_upcall dupcalls[UPCALL_MAX_BATCH]; struct upcall upcalls[UPCALL_MAX_BATCH]; struct flow flows[UPCALL_MAX_BATCH]; - size_t n_upcalls, i; + size_t n_upcalls, i, n_errors; n_upcalls = 0; + n_errors = 0; while (n_upcalls < UPCALL_MAX_BATCH) { struct ofpbuf *recv_buf = &recv_bufs[n_upcalls]; struct dpif_upcall *dupcall = &dupcalls[n_upcalls]; @@ -810,6 +811,7 @@ recv_upcalls(struct handler *handler) dupcall->type, dupcall->userdata, flow, mru, &dupcall->ufid, PMD_ID_NULL); if (error) { + n_errors++; if (error == ENODEV) { /* Received packet on datapath port for which we couldn't * associate an ofproto. This can happen if a port is removed @@ -837,6 +839,7 @@ recv_upcalls(struct handler *handler) error = process_upcall(udpif, upcall, &upcall->odp_actions, &upcall->wc); if (error) { + n_errors++; goto cleanup; } @@ -848,6 +851,13 @@ cleanup: free_dupcall: dp_packet_uninit(&dupcall->packet); ofpbuf_uninit(recv_buf); + if (n_errors >= UPCALL_MAX_BATCH * 2) { + if (n_upcalls == 0) { + return 1; + } else { + break; + } + } } if (n_upcalls) {