DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Tummala, Sivaprasad" <sivaprasad.tummala@intel.com>
To: Flavio Leitner <fbl@sysclose.org>
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>,
	"Wang, Zhihong" <zhihong.wang@intel.com>,
	"Ye, Xiaolong" <xiaolong.ye@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2] vhost: fix mbuf alloc failure
Date: Tue, 5 May 2020 05:48:46 +0000	[thread overview]
Message-ID: <FDDE0085FF3F264C97717D963DBD9EA229F3F93B@PGSMSX112.gar.corp.intel.com> (raw)
In-Reply-To: <20200504193240.GA92333@p50.lan>

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

  reply	other threads:[~2020-05-05  5:48 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-28  9:52 [dpdk-dev] [PATCH v1] vhost: fix mbuf allocation failures Sivaprasad Tummala
2020-04-29  8:43 ` Maxime Coquelin
2020-04-29 17:35   ` Flavio Leitner
2020-04-30  7:13     ` Tummala, Sivaprasad
2020-05-04 17:11 ` [dpdk-dev] [PATCH v2] vhost: fix mbuf alloc failure Sivaprasad Tummala
2020-05-04 19:32   ` Flavio Leitner
2020-05-05  5:48     ` Tummala, Sivaprasad [this message]
2020-05-05  8:20       ` Maxime Coquelin
2020-05-05 11:56         ` Tummala, Sivaprasad
2020-05-08 11:17   ` [dpdk-dev] [PATCH v3] vhost: fix mbuf allocation failures Sivaprasad Tummala
2020-05-15  7:29     ` Maxime Coquelin
2020-05-15  8:36     ` Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=FDDE0085FF3F264C97717D963DBD9EA229F3F93B@PGSMSX112.gar.corp.intel.com \
    --to=sivaprasad.tummala@intel.com \
    --cc=dev@dpdk.org \
    --cc=fbl@sysclose.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=stable@dpdk.org \
    --cc=xiaolong.ye@intel.com \
    --cc=zhihong.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).