From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-we0-f179.google.com (mail-we0-f179.google.com [74.125.82.179]) by dpdk.org (Postfix) with ESMTP id 8AED1B411 for ; Wed, 18 Feb 2015 11:33:47 +0100 (CET) Received: by wevk48 with SMTP id k48so266623wev.3 for ; Wed, 18 Feb 2015 02:33:47 -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=Fu57F6bi5a2vdgLS2VGiX5OMLjM0+qPMVLiYPvB8kKw=; b=QUh0NC+BhCbtAD5ktk0KIb04RnNONbD/OIKZXBCezGuPSuO0eyPcHRaAW/iufMv/Lt DhRBOTQkAGPTDAdJqW0LX29r8uCMFnAz+UnEzoB6diHA3JPeSBjkyzip4hoiRod0p4qP Zu5lNcUf3LyN3Q1YgdvLIROPk9iFDjDdhsI3mTuqCmbosJBGEjQMek14E7gTB52e2P98 2T6OJhD1uXb+GOGPn9jFSrv6BbGnWs53/3HsAMI39ipZDBoAk8F6gmAH4z3Mof4CVVWQ m0+CzGkgU8ZK0W9nt0sVEIxxV35rCJAZ3B+aBrqa1UvOKOLyuMaWbzY7XUlDV9TlJbNn VTpQ== X-Gm-Message-State: ALoCoQmDTt0/rxS/HkXQzSoQIKsTvVPdji5GW7OTWj5sED/g7UVh0Yfn3cVlxPLlwPA7d+sHdmM0 X-Received: by 10.180.93.226 with SMTP id cx2mr3425849wib.63.1424255627317; Wed, 18 Feb 2015 02:33:47 -0800 (PST) Received: from [10.16.0.195] (6wind.net2.nerim.net. [213.41.180.237]) by mx.google.com with ESMTPSA id j9sm31892507wjy.18.2015.02.18.02.33.46 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 18 Feb 2015 02:33:46 -0800 (PST) Message-ID: <54E46A8C.9010105@6wind.com> Date: Wed, 18 Feb 2015 11:33:48 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.2.0 MIME-Version: 1.0 To: Bruce Richardson References: <1424102913-18944-1-git-send-email-sergio.gonzalez.monroy@intel.com> <1424102913-18944-3-git-send-email-sergio.gonzalez.monroy@intel.com> <54E45888.7070603@6wind.com> <20150218093548.GA14884@bricha3-MOBL3> <2601191342CEEE43887BDE71AB977258213EF5E4@irsmsx105.ger.corp.intel.com> <20150218100003.GA14728@bricha3-MOBL3> <54E46612.7050809@6wind.com> <20150218102253.GA6804@bricha3-MOBL3> In-Reply-To: <20150218102253.GA6804@bricha3-MOBL3> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH 2/2] Remove RTE_MBUF_REFCNT references 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: Wed, 18 Feb 2015 10:33:47 -0000 Hi, On 02/18/2015 11:22 AM, Bruce Richardson wrote: > On Wed, Feb 18, 2015 at 11:14:42AM +0100, Olivier MATZ wrote: >> On 02/18/2015 11:00 AM, Bruce Richardson wrote: >>> On Wed, Feb 18, 2015 at 09:48:58AM +0000, Ananyev, Konstantin wrote: >>>> Hi lads, >>>> >>>>> -----Original Message----- >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson >>>>> Sent: Wednesday, February 18, 2015 9:36 AM >>>>> To: Olivier MATZ >>>>> Cc: dev@dpdk.org >>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Remove RTE_MBUF_REFCNT references >>>>> >>>>> On Wed, Feb 18, 2015 at 10:16:56AM +0100, Olivier MATZ wrote: >>>>>> Hi Sergio, >>>>>> >>>>>> On 02/16/2015 05:08 PM, Sergio Gonzalez Monroy wrote: >>>>>>> This patch removes all references to RTE_MBUF_REFCNT, setting the refcnt >>>>>>> field in the mbuf struct permanently. >>>>>>> >>>>>>> Signed-off-by: Sergio Gonzalez Monroy >>>>>> >>>>>> I think removing the refcount compile option goes in the right >>>>>> direction. However, activating the refcount will break the applications >>>>>> that reserve a private zone in mbufs. This is due to the macros >>>>>> RTE_MBUF_TO_BADDR() and RTE_MBUF_FROM_BADDR() that suppose that >>>>>> the beginning of the mbuf is 128 bytes (sizeof mbuf) before the >>>>>> data buffer. >>>>>> >>>>> >>>>> While I understand how the macros make certain assumptions, how does activating >>>>> the refcnt specifically lead to the problems you describe? Could you explain >>>>> that part in a bit more detail? >>>>> >>>>> Thanks, >>>>> /Bruce >>>>> >>>> >>>> Olivier, I also don't understand your concern here. >>>> As I can see, that patch has nothing to do with RTE_MBUF_FROM_BADDR/ RTE_MBUF_FROM_BADDR macros. >>>> They are still there, for example rte_pktmbuf_detach() still uses it to restore mbuf's buf_addr. >>>> The only principal change here, is that we don't rely more on RTE_MBUF_FROM_BADDR to determine, >>>> Is that indirect mbuf or not. >>>> Instead we use a special falg for that purpose: >>>> >>>> -#define RTE_MBUF_INDIRECT(mb) (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb)) >>>> +#define RTE_MBUF_INDIRECT(mb) (mb->ol_flags & IND_ATTACHED_MBUF) >>>> >>>> BTW, Sergio as I said before, I think it should be: >>>> #define RTE_MBUF_INDIRECT(mb) ((mb)->ol_flags & IND_ATTACHED_MBUF) >>>> >>>> Konstantin >>>> >>>> >>>>>> For RTE_MBUF_TO_BADDR(), it's relatively easy to replace it. The >>>>>> mbuf pool could store the size of the private size like it's done >>>>>> for mbp_priv->mbuf_data_room_size. Using rte_mempool_from_obj(m) >>>>>> or m->pool, we can retrieve the mbuf pool and this value, then >>>>>> compute the buffer address. >>> >>> Agreed, that makes sense. >>> >>>>>> >>>>>> For RTE_MBUF_FROM_BADDR(), it's more complex. We could ensure that >>>>>> a backpointer to the mbuf is always located before the data buffer, >>>>>> but it looks difficult to do. >>> >>> On the other hand, with the proposed refcnt change Sergio proposes, we no >>> longer use this macro in any of the built-in mbuf handling for freeing mbufs. >>> Does this need to be solved at anything other than the application level? >> >> It's still used in __rte_pktmbuf_prefree_seg() to retrieve the >> parent mbuf (direct) from the indirect mbuf beeing freed. >> > Yes, my bad. > How was this managed before, since refcnt field seems to be necessary in order > to effectively manage indirect mbufs? Is this just the case that this is something > that never worked and that needs to be solved, or is it something that was > working that this patch will now break? This is something that never worked before: refcounts are not compatible with reserving private data in mbufs. This patch does not change the issue, it is still there. Before the patch, an application that wanted to reserve a private data could disable refcounts at compile-time. After the patch, the solution is just to avoid using refcounts. Regards, Olivier