From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f169.google.com (mail-wi0-f169.google.com [209.85.212.169]) by dpdk.org (Postfix) with ESMTP id 139E9803F for ; Thu, 4 Dec 2014 17:03:59 +0100 (CET) Received: by mail-wi0-f169.google.com with SMTP id r20so36827249wiv.0 for ; Thu, 04 Dec 2014 08:03:58 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=L+BXRpn1PvXHOSM2Lz7mnR+/5mpfzZtTgigHcJbeYUM=; b=QypTYN6npih7ATagXFTlgSUtisWMRTVt2deCoUCPAdm6/XGkTaWRarVRaG6W9+HLr2 LQcXeHSXYmW8IFqFnwADlOZ238lgd1U3tle9OedmgelRXVNeiAqMzMkHiR9AH1GD+JhP f9aPmXGz5kbxMd7lRBGHW8gJhIufIc7/T1gGbxLrncIoIj7TGvEjeNoRxzPZNNRZq0mM blkDqVHiuW+jk1fWB0BM8/jk7Tk++JiNk4eNFzY2Wzug8qEg8lredEcPPm6o2yNhewSW qkhmGZggKlDsTzQAW8UpEE53PVa7Zs/HQgkknbqHxux541IaSxX184PDwAXYBL74yQF9 SapQ== X-Gm-Message-State: ALoCoQnpS8CBJqK9z+yAx+gXnm2/AGnaVdAf4G0uTx37A2BFLiNOc0IarvhZLYiUxhMkboSf0ahp X-Received: by 10.180.107.136 with SMTP id hc8mr12521296wib.32.1417709038762; Thu, 04 Dec 2014 08:03:58 -0800 (PST) Received: from [192.168.0.8] (crb44-1-82-67-127-5.fbx.proxad.net. [82.67.127.5]) by mx.google.com with ESMTPSA id l10sm55505975wif.20.2014.12.04.08.03.58 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 04 Dec 2014 08:03:58 -0800 (PST) Message-ID: <548085EB.4080101@6wind.com> Date: Thu, 04 Dec 2014 17:03:55 +0100 From: Jean-Mickael Guerin User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Bruce Richardson , "Ananyev, Konstantin" References: <1417703181-23093-1-git-send-email-jean-mickael.guerin@6wind.com> <1417703181-23093-3-git-send-email-jean-mickael.guerin@6wind.com> <2601191342CEEE43887BDE71AB977258213BCA29@IRSMSX105.ger.corp.intel.com> <20141204151500.GC9300@bricha3-MOBL3> <2601191342CEEE43887BDE71AB977258213BCA80@IRSMSX105.ger.corp.intel.com> <20141204153204.GD9300@bricha3-MOBL3> In-Reply-To: <20141204153204.GD9300@bricha3-MOBL3> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length 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: Thu, 04 Dec 2014 16:03:59 -0000 On 04/12/2014 16:32, Bruce Richardson wrote: > On Thu, Dec 04, 2014 at 03:29:04PM +0000, Ananyev, Konstantin wrote: >> >> >>> -----Original Message----- >>> From: Richardson, Bruce >>> Sent: Thursday, December 04, 2014 3:15 PM >>> To: Ananyev, Konstantin >>> Cc: Jean-Mickael Guerin; dev@dpdk.org >>> Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length >>> >>> On Thu, Dec 04, 2014 at 02:50:11PM +0000, Ananyev, Konstantin wrote: >>>> Hi, >>>> >>>>> -----Original Message----- >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jean-Mickael Guerin >>>>> Sent: Thursday, December 04, 2014 2:26 PM >>>>> To: dev@dpdk.org >>>>> Subject: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length >>>>> >>>>> The template mbuf_initializer is hard coded with a buflen which >>>>> might have been set differently by the application at the time of >>>>> mbuf pool creation. >>>>> >>>>> Switch to a mbuf allocation, to fetch the correct default values. >>>>> There is no performance impact because this is not a data-plane API. >>>>> >>>>> Signed-off-by: Jean-Mickael Guerin >>>>> Acked-by: David Marchand >>>>> Fixes: 0ff3324da2 ("ixgbe: rework vector pmd following mbuf changes") >>>>> --- >>>>> lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c | 19 ++++++++++++------- >>>>> 1 file changed, 12 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c >>>>> index c1b5a78..f7b02f5 100644 >>>>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c >>>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c >>>>> @@ -732,17 +732,22 @@ static struct ixgbe_txq_ops vec_txq_ops = { >>>>> int >>>>> ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq) >>>>> { >>>>> - struct rte_mbuf mb_def = { .buf_addr = 0 }; /* zeroed mbuf */ >>>>> + struct rte_mbuf *mb_def; >>>>> >>>>> - mb_def.nb_segs = 1; >>>>> - mb_def.data_off = RTE_PKTMBUF_HEADROOM; >>>>> - mb_def.buf_len = rxq->mb_pool->elt_size - sizeof(struct rte_mbuf); >>>>> - mb_def.port = rxq->port_id; >>>>> - rte_mbuf_refcnt_set(&mb_def, 1); >>>>> + mb_def = rte_pktmbuf_alloc(rxq->mb_pool); >>>> >>>> Could you explain to me, what is an advantage of using dynamic allocation vs local struct here? >>>> I don't see any. >>> >>> It means that we get an mbuf that is initialized as done by the initialization >>> function passed to the mempool_create call. The static variable method assumes >>> that we configure the mbuf using default setting for buf_len etc. >>> >> >> I understand that, but why it can't be done in some other way? >> Without allocating/freeing? >> Let say, at mempool_create() store obj_init() and then add ability to call it again? This is a good idea, useful in other places of mbuf API. >> Anyway, it doesn't look to me like a critical problem, that requires an urgent patch for 1.8. >> This is about data corruption - a simple function like rte_pktmbuf_tailroom() returns an incorrect value... Let me try obj_init() variant and we will see if it is acceptable in 1.8 - it does not look a big change after all. >>>> Plus if rte_pktmbuf_alloc() would fail, we'll leave our rx queue not configured properly. >>>> As I can see ixgbe_dev_rx_queue_setup() doesn't check the return value of > ixgbe_rxq_vec_setup() >>>> (as it is just not supposed to fail). >>>> So ixgbe_dev_rx_queue_setup() will return OK for unconfigured RX queue. >>> >>> Good catch, that's something that should perhaps be looked at in a V2 patch, I >>> think. >>> >>>> >>>>> + if (mb_def == NULL) { >>>>> + PMD_INIT_LOG(ERR, "ixgbe_rxq_vec_setup: could not allocate one mbuf"); >>>>> + return -1; >>>>> + } >>>>> + /* nb_segs, refcnt, data_off and buf_len are already set */ >>>>> + mb_def->port = rxq->port_id; >>>>> >>>>> /* prevent compiler reordering: rearm_data covers previous fields */ >>>>> rte_compiler_barrier(); >>>> >>>> I don't think we need it here. >>> >>> I think we might, as the compiler doesn't know that the rearm data overlaps >>> with the previously set fields, so may reorder the reads and writes thinking >>> they are independent. >> >> Why it doesn't? >> I suppose compiler has all the knowledge of the mbuf structure layout at that point. >> Or there is a some sort of bug in some version of the compiler? >> > > No, we're just violating the layout here by dereferencing past the end of the array :-) > > /Bruce > >>>> >>>>> - rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data); >>>>> + rxq->mbuf_initializer = *((uint64_t *)&mb_def->rearm_data); >>>>> + >>>>> + rte_pktmbuf_free(mb_def); >>>>> + >>>>> return 0; >>>>> } >>>>> >>>>> -- >>>>> 2.1.3 >>>> >>>> Somy vote - NACK for the whole series. >>>> Konstantin >>>>