Re: [UNIKRAFT/LIBLWIP PATCH v2 3/3] lib/lwip: Enable poll only receive

[Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>]

Hi Sharan,

tested, reviewed, looks good to me. Small typo in uknetdev_updown.
Also, some lines are more than 80 characters. I don't really care, but
checkpatch will probably complain.

Overall the series looks almost fit for upstreaming in my opinion. The
v3 will be good I think.

regards,
Hugo

On Tue, 2020-06-30 at 13:15 +0200, Sharan Santhanam wrote:
> The uknetdev library provides the way to check if interrupts are
> supported on a uk_netdev. If the device does not support interrupt
> the lwip stack would create a thread to poll the receive queue for
> packets.
> 
> Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> ---
>  uknetdev.c | 91 ++++++++++++++++++++++++++++++++++++++++----------
> ----
>  1 file changed, 68 insertions(+), 23 deletions(-)
> 
> diff --git a/uknetdev.c b/uknetdev.c
> index ef57014..0831204 100644
> --- a/uknetdev.c
> +++ b/uknetdev.c
> @@ -64,6 +64,11 @@
>  
>  struct lwip_netdev_data {
>       uint32_t features;
> +#ifdef CONFIG_HAVE_SCHED
> +     struct uk_thread *poll_thread; /* Thread per device */
> +     char *_name; /* Thread name */
> +     struct uk_sched *sched; /* Scheduler information */
> +#endif /* CONFIG_HAVE_SCHED */
>  };
>  
>  /*
> @@ -325,44 +330,84 @@ void uknetdev_poll_all(void)
>  
>  #else /* CONFIG_LWIP_NOTHREADS */
>  
> +static void _poll_netif(void *arg)
> +{
> +     struct netif *nf = (struct netif *) arg;
> +
> +     while (1) {
> +             uknetdev_poll(nf);
> +             uk_sched_yield();
> +     }
> +}
> +
>  static void uknetdev_updown(struct netif *nf)
>  {
>       struct uk_netdev *dev;
>       int ret;
> +     struct lwip_netdev_data  *lwip_data;
>  
>       UK_ASSERT(nf);
>       dev = netif_to_uknetdev(nf);
>       UK_ASSERT(dev);
> +     lwip_data = (struct lwip_netdev_data *)dev->scratch_pad;
>  
>       /* Enable and disable interrupts according to netif's
> up/down status */
> +
>       if (nf->flags & NETIF_FLAG_UP) {
> -             ret = uk_netdev_rxq_intr_enable(dev, 0);
> -             if (ret < 0) {
> -                     LWIP_DEBUGF(NETIF_DEBUG,
> -                                 ("%s: %c%c%u: Failed to enable
> rx interrrupt mode on netdev %u\n",
> -                                  __func__, nf->name[0], nf-
> >name[1],
> -                                  nf->num,
> uk_netdev_id_get(dev)));
> +             if (uk_netdev_rxintr_supported(lwip_data->features)) 
> {
> +                     ret = uk_netdev_rxq_intr_enable(dev, 0);
> +                     if (ret < 0) {
> +                             LWIP_DEBUGF(NETIF_DEBUG,
> +                                             ("%s: %c%c%u: Failed
> to enable rx interrupt mode on netdev %u\n",
> +                                              __func__, nf-
> >name[0],
> +                                              nf->name[1],
> +                                              nf->num,
> +                                              uk_netdev_id_get(de
> v)));
> +                     } else {
> +                             LWIP_DEBUGF(NETIF_DEBUG,
> +                                     ("%s: %c%c%u: Enabled rx
> interrupt mode on netdev %u\n",
> +                                              __func__, nf-
> >name[0],
> +                                              nf->name[1],
> +                                              nf->num,
> +                                              uk_netdev_id_get(de
> v)));
> +                     }
> +
> +                     if (ret == 1) {
> +                             /*
> +                              * uk_netdev_rxq_intr_enable() told
> us that we
> +                              * need to flush the receieve queue
> before

s/receieve/receive/

> +                              * interrupts are enabled. For this
> purpose
> +                              * we do an initial poll.
> +                              */
> +                             uknetdev_poll(nf);
> +                     }
>               } else {
> +#ifdef CONFIG_HAVE_SCHED
>                       LWIP_DEBUGF(NETIF_DEBUG,
> -                                 ("%s: %c%c%u: Enabled rx
> interrupt mode on netdev %u\n",
> -                                  __func__, nf->name[0], nf-
> >name[1],
> -                                  nf->num,
> uk_netdev_id_get(dev)));
> -             }
> -
> -             if (ret == 1) {
> -                     /*
> -                      * uk_netdev_rxq_intr_enable() told us that
> we need to
> -                      * flush the receieve queue before
> interrupts are
> -                      * enabled. For this purpose we do an
> initial poll.
> -                      */
> -                     uknetdev_poll(nf);
> +                                     ("%s: Poll receive
> enabled\n",
> +                                      __func__));
> +                     /* Create a thread */
> +                     lwip_data->sched = uk_sched_get_default();
> +                     UK_ASSERT(lwip_data->sched);
> +                     lwip_data->poll_thread =
> +                             uk_sched_thread_create(lwip_data-
> >sched, NULL,
> +                                                    NULL,
> _poll_netif, nf);
> +#else /* CONFIG_HAVE_SCHED */
> +                     uk_pr_warn("The netdevice does not support
> interrupt. Ensure the netdevice is polled to receive packets");
> +#endif /* CONFIG_HAVE_SCHED */
>               }
>       } else {
> -             uk_netdev_rxq_intr_disable(dev, 0);
> -             LWIP_DEBUGF(NETIF_DEBUG,
> -                         ("%s: %c%c%u: Disabled rx interrupts on
> netdev %u\n",
> -                          __func__, nf->name[0], nf->name[1],
> -                          nf->num, uk_netdev_id_get(dev)));
> +             /**
> +              * TODO:
> +              * Cleanup the thread on stopping the network
> interface.
> +              */
> +             if (uk_netdev_rxintr_supported(lwip_data->features)) 
> {
> +                     uk_netdev_rxq_intr_disable(dev, 0);
> +                     LWIP_DEBUGF(NETIF_DEBUG,
> +                                     ("%s: %c%c%u: Disabled rx
> interrupts on netdev %u\n",
> +                                      __func__, nf->name[0], nf-
> >name[1],
> +                                      nf->num,
> uk_netdev_id_get(dev)));
> +             }
>  
>       }
>  }