From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f179.google.com (mail-wi0-f179.google.com [209.85.212.179]) by dpdk.org (Postfix) with ESMTP id A2E235A41 for ; Thu, 9 Apr 2015 15:06:33 +0200 (CEST) Received: by wizk4 with SMTP id k4so91575071wiz.1 for ; Thu, 09 Apr 2015 06:06:33 -0700 (PDT) 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=YwnjSgj8pqli9c3Fk1pSs/Ps5hnQ1El3E6PZX8XPl9c=; b=mSeL16XegNRbOEvi7sEaEJr70ThVSBs3Xn8y9F3lRsZhZoIm6/2r9thN0cMe2DmoB5 jHP0kKHeChjgZNTJGvMGgx/PKvTUhh0VfsLojvwJ6IxLOP5k+Vpn2DoveSUd1axzbMNR IUT4aCJN1ieM3u5DL5wiA7ia18tb9W6+hssg+pVNANvZROyE7p9z9mXSFvXA2X23vMcN 35JWbLmm/2Oz1rYGFJGpkQv5CURZmGFQiW9l3/+gYQanDyofoxGvN/A9rU63Zn2otke+ +JPb4wYtInNtOZ6ze1atOHeldQ0aojNDFJLb/CQqXKXch/sZuyeSS8g57iKe9hqAg6ip +/JQ== X-Gm-Message-State: ALoCoQl2xD/KvcjQ93Kc1qLsm1oi4gqqaBgxMunA2vEPdPsTWOuxzYgpSH/A/faVeg6/5Ts78Sy8 X-Received: by 10.180.211.2 with SMTP id my2mr5981302wic.78.1428584793468; Thu, 09 Apr 2015 06:06:33 -0700 (PDT) Received: from [10.16.0.195] (6wind.net2.nerim.net. [213.41.180.237]) by mx.google.com with ESMTPSA id eh5sm5471155wic.20.2015.04.09.06.06.31 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 09 Apr 2015 06:06:32 -0700 (PDT) Message-ID: <55267956.6020504@6wind.com> Date: Thu, 09 Apr 2015 15:06:30 +0200 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.3.0 MIME-Version: 1.0 To: "Ananyev, Konstantin" , "dev@dpdk.org" References: <1427385595-15011-1-git-send-email-olivier.matz@6wind.com> <1427829784-12323-1-git-send-email-zer0@droids-corp.org> <1427829784-12323-2-git-send-email-zer0@droids-corp.org> <2601191342CEEE43887BDE71AB97725821413A2D@irsmsx105.ger.corp.intel.com> <5522FF6B.1030503@6wind.com> <2601191342CEEE43887BDE71AB97725821414310@irsmsx105.ger.corp.intel.com> <5523FB9B.2060508@6wind.com> <2601191342CEEE43887BDE71AB9772582141451F@irsmsx105.ger.corp.intel.com> <5524F876.8090900@6wind.com> <2601191342CEEE43887BDE71AB97725821414805@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725821414805@irsmsx105.ger.corp.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data 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, 09 Apr 2015 13:06:33 -0000 Hi Konstantin, On 04/08/2015 03:45 PM, Ananyev, Konstantin wrote: > Hi Olivier, > >> -----Original Message----- >> From: Olivier MATZ [mailto:olivier.matz@6wind.com] >> Sent: Wednesday, April 08, 2015 10:44 AM >> To: Ananyev, Konstantin; dev@dpdk.org >> Cc: zoltan.kiss@linaro.org; Richardson, Bruce >> Subject: Re: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data >> >> Hi Konstantin, >> >> On 04/07/2015 07:17 PM, Ananyev, Konstantin wrote: >>>> Just to be sure we're on the same line: >>>> >>>> - before the patch series >>>> >>>> - private area was working before that patch series if clones were not >>>> used. To use a private are, the user had to provide another >>>> function derived from pktmbuf_init() to change m->buf_addr and >>>> m->buf_len. >>>> - using both private area + clones was broken >>>> >>>> - after the patch series >>>> >>>> - private area is working with or without clone. But yo use it, >>>> the user still has to provide another function to change >>>> m->buf_addr, m->buf_len *and m->priv_size*. >>>> >>>> The series just fixes the fact that "clones + priv" was not working. >>>> It does not address the problem that providing a new pktmbuf_init() >>>> function is required to use privata area. To fix this, I think it >>>> could require a API evolution that should be part of another series. >>> >>> I don't think we need new pktmbuf_init(). >>> We just need to update it, so both pktmbuf_init() and detach() setup >>> buf_addr, buf_len (and priv_size) to exactly the same values. >>> If they don't do that, it means that you can't use attach/detach with >>> mempools created with pktmbuf_init() any more. >>> >>> BTW, another thing that I just realised: >>> examples/ipv4_multicast and examples/ip_fragmentation/ - >>> both create a pool of mbufs with elem_size < 2K and don't populate mempool's private area - >>> so mbp_priv->mbuf_data_room_size == 0, for them. >>> >>> So that code in detach(): >>> >>> + mbp_priv = rte_mempool_get_priv(mp); >>> + m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM - >>> + mbp_priv->mbuf_data_room_size - >>> + sizeof(struct rte_mbuf); >>> >>> >>> Would break both these samples. >>> I suppose we need to handle situation when mp->elt_size < RTE_PKTMBUF_HEADROOM + sizeof(struct rte_mbuf), >>> (and probably also when mbuf_data_room_size == 0) correctly. >> >> Indeed. I think a mbuf pool (even with buf_len == 0 like in >> ip_fragmentation example) should have a pool with a private area and >> should call rte_pktmbuf_pool_init() to populate it. So >> rte_pktmbuf_pool_init() has to be fixed first to use elt_size >> and support the buf_len < RTE_PKTMBUF_HEADROOM, then we can >> update frag/multicast examples. >> >> Unfortunately, we don't know the size of the mbuf private area >> in rte_pktmbuf_pool_init() if the opaque arg (data_room_size) >> is 0, which is the default. I think it should be replaced by a structure >> containing data_room_size and mbuf_priv_size, but it would break >> applications that are setting data_room_size. > > Yes, same thoughts here. > >> I don't see any good >> solution to do that while keeping a backward compatibility for >> rte_pktmbuf_pool_init(), but as the current API is not ideal, >> I think it's worth changing it and add something in the release >> note. > > If no one else has a better alternative than that, then I suppose it is good enough. > >> >> We may also want to introduce a new helper as discussed previously: >> >> struct rte_mempool * >> rte_pktmbuf_pool_create(const char *name, unsigned n, unsigned elt_size, >> unsigned cache_size, size_t mbuf_priv_size, >> rte_mempool_obj_ctor_t *obj_init, void *obj_init_arg, >> int socket_id, unsigned flags) >> >> Any comment? > > Looks good to me. > Should we also introduce rte_pktmbuf_pool_xmem_create()? Hmmm as it's only used in one place, I'm not sure it's really required. As the previous way to create mbuf pool using rte_mempool_create() will still work, I think we could consider it's a special case. If it becomes necessary, there's no problem to add it later. Olivier > Konstantin > >> >> >>> >>> Konstantin >