On Thu, Feb 1, 2024 at 9:29 AM Maxime Coquelin
<maxime.coque...@redhat.com> wrote:
> On 2/1/24 09:10, David Marchand wrote:
> > On Wed, Jan 31, 2024 at 8:53 PM Maxime Coquelin
> > <maxime.coque...@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.coque...@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?
>
> Agree, we should not have such log in the datapath.
> And now that we have the stat, it is even less useful.
Ok, nevertheless, you can add my:
Reviewed-by: David Marchand <david.march...@redhat.com>
--
David Marchand
