Re: [PATCH v2 2/2] vhost: add new mbuf allocation failure statistic

[David Marchand <david.marchand@redhat.com>]

On Wed, Jan 31, 2024 at 8:53 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch introduces a new, per virtqueue, mbuf allocation
> failure statistic. It can be useful to troubleshoot packets
> drops due to insufficient mempool size or memory leaks.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Having a stat for such situation will be useful.

I just have one comment, though it is not really related to this change itself.

[snip]

> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 9951842b9f..1359c5fb1f 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -2996,6 +2996,7 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
>                 if (mbuf_avail == 0) {
>                         cur = rte_pktmbuf_alloc(mbuf_pool);
>                         if (unlikely(cur == NULL)) {
> +                               vq->stats.mbuf_alloc_failed++;
>                                 VHOST_DATA_LOG(dev->ifname, ERR,
>                                         "failed to allocate memory for mbuf.");

This error log here is scary as it means the datapath can be slowed
down for each multisegment mbuf in the event of a mbuf (maybe
temporary) shortage.
Besides no other mbuf allocation in the vhost library datapath
generates such log.

I would remove it, probably in a separate patch.
WDYT?


>                                 goto error;


-- 
David Marchand