[ovs-dev] [PATCH] upcall: avoid handler block in recv_upcalls

[Eli Britstein]

On 12/26/2020 11:51 AM, wangyunjian wrote:
> From: Lvmengfan <lvmengfan at 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 at 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) {