From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 02647A04AE; Tue, 5 May 2020 07:48:59 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CC6661D54C; Tue, 5 May 2020 07:48:58 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id A94361D52D; Tue, 5 May 2020 07:48:54 +0200 (CEST) IronPort-SDR: v782aYDNoUdlhAVBSJgjEOOFwsSUisq0xtMrIbClUvFe3IV1mLovMoypyjA0RhhWfMs4s1Z8g7 VT9qWlono6Uw== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 May 2020 22:48:54 -0700 IronPort-SDR: jcKlvcLlWsfwvYZPTjV5l1y11K27dWuS6sITnBNBDFF9B6awPBzJAzAmHXZTq/IcqS4VbzpbDz 2L9rXgS2pHyw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,354,1583222400"; d="scan'208,217";a="369312000" Received: from pgsmsx111.gar.corp.intel.com ([10.108.55.200]) by fmsmga001.fm.intel.com with ESMTP; 04 May 2020 22:48:52 -0700 Received: from pgsmsx112.gar.corp.intel.com ([169.254.3.217]) by PGSMSX111.gar.corp.intel.com ([169.254.2.69]) with mapi id 14.03.0439.000; Tue, 5 May 2020 13:48:47 +0800 From: "Tummala, Sivaprasad" To: Flavio Leitner CC: Maxime Coquelin , "Wang, Zhihong" , "Ye, Xiaolong" , "dev@dpdk.org" , "stable@dpdk.org" Thread-Topic: [PATCH v2] vhost: fix mbuf alloc failure Thread-Index: AQHWIkrTpvfY9vJu+kqieVUVJg9WoaiY4aOA Date: Tue, 5 May 2020 05:48:46 +0000 Message-ID: References: <20200428095203.64935-1-Sivaprasad.Tummala@intel.com> <20200504171118.93782-1-Sivaprasad.Tummala@intel.com> <20200504193240.GA92333@p50.lan> In-Reply-To: <20200504193240.GA92333@p50.lan> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZTA4Zjg4MDQtNjQxYS00YjI4LWI1M2UtOTE5ODkzMjNhMDc3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiUDdLQnQwZ0FMMStQK3NOTXZrYmcxbldaMExCK2NtZUZiNVNFUE5SYVpcL0JwOFVMSDZCZDEycXl1OWVQcTZRNTMifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [172.30.20.206] MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2] vhost: fix mbuf alloc failure X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Flavio, Thanks for your comments. SNIPPED > pkts[i] =3D virtio_dev_pktmbuf_alloc(dev, mbuf_pool,= buf_len); > - if (unlikely(pkts[i] =3D=3D NULL)) > + if (unlikely(pkts[i] =3D=3D NULL)) { > + /* > + * mbuf allocation fails for jumbo packets = when external > + * buffer allocation is not allowed and lin= ear 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 enab= led. 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 li= mit by using a static bool: static bool allocerr_warned =3D false; if (allocerr_warned) { VHOST_LOG_DATA(ERR, "Failed to allocate memory for mbuf. Packet dropped!\n"); allocerr_warned =3D 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 mechan= ism, 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 *de= v, > struct rte_mbuf **pkts) > { > > - uint16_t buf_id, desc_count; > + uint16_t buf_id, desc_count =3D 0; > + int ret; > > - if (vhost_dequeue_single_packed(dev, vq, mbuf_pool, pkts, &buf_id= , > - &desc_count)) > - return -1; > + ret =3D 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_c= ount); > + if (likely(desc_count > 0)) { The vhost_dequeue_single_packed() could return -1 with desc_count > 0 and t= his change doesn't handle that. Yes, as per my current understanding in either success or failure we need t= o 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