From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx.bisdn.de (mx.bisdn.de [185.27.182.31]) by dpdk.org (Postfix) with ESMTP id 23BE86A9B for ; Thu, 9 Oct 2014 10:37:37 +0200 (CEST) Received: from [192.168.1.101] (f053064184.adsl.alicedsl.de [78.53.64.184]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx.bisdn.de (Postfix) with ESMTPSA id 93C4DA2700; Thu, 9 Oct 2014 10:44:57 +0200 (CEST) Message-ID: <54364B1F.8030605@bisdn.de> Date: Thu, 09 Oct 2014 10:45:19 +0200 From: Marc Sune User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20131103 Icedove/17.0.10 MIME-Version: 1.0 To: "Zhang, Helin" References: <1411985756-2744-1-git-send-email-marc.sune@bisdn.de> <543633B5.3020101@bisdn.de> <54363ED5.9060304@bisdn.de> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] KNI: use a memzone pool for KNI alloc/release 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 Oct 2014 08:37:37 -0000 Hi Helin, Inline and snipped. Thanks for the additional comments. On 09/10/14 10:33, Zhang, Helin wrote: > [snip] >>> [snip] >>>>>> It adds a new API call, rte_kni_init(max_kni_ifaces) that shall be >>>>>> called before any call to rte_kni_alloc() if KNI is used. >>> To avoid the additional interface, this initialization works can be >>> done during the first time of calling rte_kni_alloc(), please refer to how it >> opens kni_fd ("/dev/kni"). >>> Also I think there should be some de-initialization works should be done in >> rte_kni_close(). >> How is rte_kni_alloc() supposed to know the size of the pool that has to be >> pre-allocated (max_kni_ifaces)? > Add it into 'struct rte_kni_conf', also a default one might be needed if 0 is > configured by the user app. I disagree with this approach :) . struct rte_kni_conf is a per-interface configuration struct, and the mempool is shared between all the alloc/release of the KNI interfaces. I don't like the approach to mix one-time-use (first alloc) parameters that affect the entire KNI system into the struct rte_kni_conf. >> I don't think the approach of pre-allocating on the first >> rte_kni_alloc() would work (I already discarded this approach before >> implementing the patch), because this would imply we need a define of #define >> MAX_KNI_IFACES during compilation time of DPDK, and the pre-allocation is >> highly dependent on the amount of hugepages memory you have and the usage >> of the KNI interfaces the applications wants to do. >> We can easily end up with DPDK users having to tweak the default >> MAX_KNI_IFACES before compiling DPDK every time, which is definetely not >> desirable IMHO. > Your idea is good! My point is it possible to avoid adding new interface, then no > changes are needed in user app. I see the current approach the most clean and comprehensive (from the perspective of the user of the library) approach. Do you have any other proposal? I am open to discuss and eventually implement it if it turns out to be better. > >> For rte_kni_close(), the pool is static (incl. the slot struct), and the memzones >> cannot be unreserved, hence there is nothing AFAIU to de-initialize; what do >> you mean specifically? > You can see that rte_kni_close() will be called in XEN (#ifdef RTE_LIBRTE_XEN_DOM0), > XEN support is different from standard Linux support. OK it is called, but what is the (extra) state that I should de-initialize that is coming from this patch? I cannot see any state I've added I have to de-initialize here. Many thanks Marc