Re: [Minios-devel] [UNIKRAFT PATCH v3 3/4] drivers/virtio: virtio_netdev_recv(): Remove disabling interrupts

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Simon,

This patch is fine.

Reviewed-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>

Thanks & Regards
Sharan

On 1/31/19 1:04 AM, Simon Kuenzer wrote:
> Disabling interrupts by virtio_netdev_recv() is not needed because in
> normal cases they are already disabled by the internal receive
> interrupt handler, right before a uknetdev API program can call the
> receive function. When the device is used in pure polling mode, queue
> interrutps are never enabled.
> We remove this step because of performance reasons. An assertion is
> making sure that the API is used as intended.
>
> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> ---
>   lib/uknetdev/include/uk/netdev.h | 8 +++++++-
>   plat/drivers/virtio/virtio_net.c | 6 ++----
>   2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/lib/uknetdev/include/uk/netdev.h b/lib/uknetdev/include/uk/netdev.h
> index 88b89135..6b17b469 100644
> --- a/lib/uknetdev/include/uk/netdev.h
> +++ b/lib/uknetdev/include/uk/netdev.h
> @@ -422,7 +422,13 @@ static inline int uk_netdev_rxq_intr_disable(struct 
> uk_netdev *dev,
>   }
>   
>   /**
> - * Receive one packet and re-program used receive descriptors
> + * Receive one packet and re-program used receive descriptors. In order to 
> avoid
> + * race conditions, queue interrupts have to be off while executing this
> + * function. When operating the queue in interrupt mode, this is automatically
> + * the case as soon as an interrupt arrived or the return code of
> + * uk_netdev_rxq_intr_enable() indicated that packets are left on the queue.
> + * In both cases, uk_netdev_rx_one() is going to enable interrupts again as 
> soon
> + * as the last packet was received from the queue.
>    * If this function is called from interrupt context (e.g., within receive 
> event
>    * handler when no dispatcher threads are configured) make sure that the
>    * provided receive buffer allocator function is interrupt-context-safe
> diff --git a/plat/drivers/virtio/virtio_net.c b/plat/drivers/virtio/virtio_net.c
> index cb771efe..c8760a05 100644
> --- a/plat/drivers/virtio/virtio_net.c
> +++ b/plat/drivers/virtio/virtio_net.c
> @@ -544,10 +544,8 @@ static int virtio_netdev_recv(struct uk_netdev *dev,
>         UK_ASSERT(dev && queue);
>         UK_ASSERT(pkt);
>   
> -	if (queue->intr_enabled & VTNET_INTR_USR_EN_MASK) {
> -               virtqueue_intr_disable(queue->vq);
> -               queue->intr_enabled &= ~(VTNET_INTR_EN);
> -       }
> +       /* Queue interrupts have to be off when calling receive */
> +       UK_ASSERT(!(queue->intr_enabled & VTNET_INTR_EN));
>   
>   	rc = virtio_netdev_rxq_dequeue(queue, pkt);
>         if (unlikely(rc < 0)) {
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel