From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id CD1332C47 for ; Mon, 7 Mar 2016 04:00:00 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 06 Mar 2016 19:00:01 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,549,1449561600"; d="scan'208";a="918242693" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga001.fm.intel.com with ESMTP; 06 Mar 2016 19:00:00 -0800 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 6 Mar 2016 18:59:58 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.110.14) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 6 Mar 2016 18:59:58 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.136]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.24]) with mapi id 14.03.0248.002; Mon, 7 Mar 2016 10:59:56 +0800 From: "Xie, Huawei" To: Yuanhan Liu Thread-Topic: [PATCH v2 1/7] vhost: refactor rte_vhost_dequeue_burst Thread-Index: AdF1agd9clXhimYJTx2/yKoyh8rOiw== Date: Mon, 7 Mar 2016 02:59:55 +0000 Message-ID: References: <1449122773-25510-1-git-send-email-yuanhan.liu@linux.intel.com> <1455803352-5518-1-git-send-email-yuanhan.liu@linux.intel.com> <1455803352-5518-2-git-send-email-yuanhan.liu@linux.intel.com> <20160304021705.GT14300@yliu-dev.sh.intel.com> <20160307024838.GY14300@yliu-dev.sh.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "Michael S. Tsirkin" , "dev@dpdk.org" , Victor Kaplansky Subject: Re: [dpdk-dev] [PATCH v2 1/7] vhost: refactor rte_vhost_dequeue_burst X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Mar 2016 03:00:01 -0000 On 3/7/2016 10:47 AM, Yuanhan Liu wrote:=0A= > On Mon, Mar 07, 2016 at 02:32:46AM +0000, Xie, Huawei wrote:=0A= >> On 3/4/2016 10:15 AM, Yuanhan Liu wrote:=0A= >>> On Thu, Mar 03, 2016 at 04:30:42PM +0000, Xie, Huawei wrote:=0A= >>>> On 2/18/2016 9:48 PM, Yuanhan Liu wrote:=0A= >>>>> + mbuf_avail =3D 0;=0A= >>>>> + mbuf_offset =3D 0;=0A= >>>>> + while (desc_avail || (desc->flags & VRING_DESC_F_NEXT) !=3D 0) {=0A= >>>>> + /* This desc reachs to its end, get the next one */=0A= >>>>> + if (desc_avail =3D=3D 0) {=0A= >>>>> + desc =3D &vq->desc[desc->next];=0A= >>>>> +=0A= >>>>> + desc_addr =3D gpa_to_vva(dev, desc->addr);=0A= >>>>> + rte_prefetch0((void *)(uintptr_t)desc_addr);=0A= >>>>> +=0A= >>>>> + desc_offset =3D 0;=0A= >>>>> + desc_avail =3D desc->len;=0A= >>>>> +=0A= >>>>> + PRINT_PACKET(dev, (uintptr_t)desc_addr, desc->len, 0);=0A= >>>>> + }=0A= >>>>> +=0A= >>>>> + /*=0A= >>>>> + * This mbuf reachs to its end, get a new one=0A= >>>>> + * to hold more data.=0A= >>>>> + */=0A= >>>>> + if (mbuf_avail =3D=3D 0) {=0A= >>>>> + cur =3D rte_pktmbuf_alloc(mbuf_pool);=0A= >>>>> + if (unlikely(!cur)) {=0A= >>>>> + RTE_LOG(ERR, VHOST_DATA, "Failed to "=0A= >>>>> + "allocate memory for mbuf.\n");=0A= >>>>> + if (head)=0A= >>>>> + rte_pktmbuf_free(head);=0A= >>>>> + return NULL;=0A= >>>>> + }=0A= >>>> We could always allocate the head mbuf before the loop, then we save t= he=0A= >>>> following branch and make the code more streamlined.=0A= >>>> It reminds me that this change prevents the possibility of mbuf bulk= =0A= >>>> allocation, one solution is we pass the head mbuf from an additional= =0A= >>>> parameter.=0A= >>> Yep, that's also something I have thought of.=0A= >>>=0A= >>>> Btw, put unlikely before the check of mbuf_avail and checks elsewhere.= =0A= >>> I don't think so. It would benifit for the small packets. What if,=0A= >>> however, when TSO or jumbo frame is enabled that we have big packets?= =0A= >> Prefer to favor the path that packet could fit in one mbuf.=0A= > Hmmm, why? While I know that TSO and mergeable buf is disable by default= =0A= > in our vhost example vhost-switch, they are enabled by default in real=0A= > life.=0A= =0A= mergable is only meaningful in RX path.=0A= If you mean big packets in TX path, i personally favor the path that=0A= packet fits in one mbuf.=0A= =0A= =0A= >> Btw, not specially to this, return "m =3D copy_desc_to_mbuf(dev, vq,=0A= >> desc_indexes[i], mbuf_pool)", failure case is unlikely to happen, so add= =0A= >> unlikely for the check if (m =3D=3D NULL) there. Please check all branch= es=0A= >> elsewhere.=0A= > Thanks for the remind, will have a detail check.=0A= >=0A= > --yliu=0A= >=0A= =0A=