DPDK patches and discussions
 help / color / mirror / Atom feed
From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: "Tummala, Sivaprasad" <sivaprasad.tummala@intel.com>,
	Flavio Leitner <fbl@sysclose.org>
Cc: "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 10:20:56 +0200
Message-ID: <cb6a1e09-e6fb-f2e4-8d9b-3b22c60ea398@redhat.com> (raw)
In-Reply-To: <FDDE0085FF3F264C97717D963DBD9EA229F3F93B@PGSMSX112.gar.corp.intel.com>

(Please try to avoid HTML for the replies, it makes it hard to follow)

See my replies below:

On 5/5/20 7:48 AM, Tummala, Sivaprasad wrote:
> 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`.

I prefer Flavio's proposal, it would have less performance impact than
increasing struct virtio_net size. As soon as we can see the error
popping once in the log message, it gives some clues on what to
investigate. Maybe providing more details on the failure could help,
like printing the pool name and the requested length.

Maxime


  reply	other threads:[~2020-05-05  8:21 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
2020-05-05  8:20       ` Maxime Coquelin [this message]
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=cb6a1e09-e6fb-f2e4-8d9b-3b22c60ea398@redhat.com \
    --to=maxime.coquelin@redhat.com \
    --cc=dev@dpdk.org \
    --cc=fbl@sysclose.org \
    --cc=sivaprasad.tummala@intel.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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git