Re: [dpdk-stable] [PATCH v2] vhost: fix mbuf alloc failure

["Tummala, Sivaprasad" <sivaprasad.tummala@intel.com>]

Hi Flavio,



Thanks for your comments.



SNIPPED



>                      pkts[i] = virtio_dev_pktmbuf_alloc(dev, mbuf_pool, buf_len);

> -                   if (unlikely(pkts[i] == NULL))

> +                  if (unlikely(pkts[i] == NULL)) {

> +                              /*

> +                              * mbuf allocation fails for jumbo packets when external

> +                              * buffer allocation is not allowed and linear buffer

> +                              * is required. Drop this packet.

> +                              */

> +#ifdef RTE_LIBRTE_VHOST_DEBUG

> +                              VHOST_LOG_DATA(ERR,

> +                                          "Failed to allocate memory for mbuf. Packet dropped!\n"); #endif



That message is useful to spot that missing packets that happens once in a while, so we should be able to see it even in production without debug enabled. However, we can't let it flood the log.

Agreed, but VHOST_LOG wrapper does not have rate limit functionality.





I am not sure if librte eal has this functionality, but if not you could limit by using a static bool:



static bool allocerr_warned = false;



if (allocerr_warned) {

    VHOST_LOG_DATA(ERR,

    "Failed to allocate memory for mbuf. Packet dropped!\n");

    allocerr_warned = true;

}



This is good idea, but having a static variable makes it file scope making it to  entire VHOST devices. Hence if the intention is to implement device specific

log rate limit, should not we resort to `dev->allocerr_warn` counter mechanism, which resets after n failures `#define LOG_ALLOCFAIL 32`.



SNIPPED



>  static __rte_always_inline int

> @@ -1946,21 +1967,24 @@ virtio_dev_tx_single_packed(struct virtio_net *dev,

>                                      struct rte_mbuf **pkts)

>  {

>

> -       uint16_t buf_id, desc_count;

> +      uint16_t buf_id, desc_count = 0;

> +      int ret;

>

> -       if (vhost_dequeue_single_packed(dev, vq, mbuf_pool, pkts, &buf_id,

> -                                                       &desc_count))

> -                   return -1;

> +      ret = vhost_dequeue_single_packed(dev, vq, mbuf_pool, pkts, &buf_id,

> +                                                      &desc_count);

>

> -       if (virtio_net_is_inorder(dev))

> -                   vhost_shadow_dequeue_single_packed_inorder(vq, buf_id,

> -                                                                                  desc_count);

> -       else

> -                   vhost_shadow_dequeue_single_packed(vq, buf_id, desc_count);

> +      if (likely(desc_count > 0)) {





The vhost_dequeue_single_packed() could return -1 with desc_count > 0 and this change doesn't handle that.


Yes, as per my current understanding in either success or failure we need to flush the descriptors `desc_count` to handle this issue.
Is there an expectation  for partial or incomplete packet where `num_desc` is greater than 0, we need to preserve it.


SNIPPED



Thanks & Regards,

Sivaprasad