From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f44.google.com (mail-wm0-f44.google.com [74.125.82.44]) by dpdk.org (Postfix) with ESMTP id DCBB92BD3 for ; Fri, 23 Mar 2018 10:31:37 +0100 (CET) Received: by mail-wm0-f44.google.com with SMTP id v21so2326530wmc.1 for ; Fri, 23 Mar 2018 02:31:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=S7wslpAiCNAIoynt+t12sCGV+5ndiJBrxNLdKN+Du8I=; b=cVn2YliFuifBxWg0e3VTq233HmFewbLsDbVXmtFg9J9g450BtTa/A7VAHfULhF9sWv ZmzifKvCals4Qg5y1TJS4dCwmB0MUiTm/HzNu8mWgYvJfLMc4m2f9tVnvLjCm9m6qeOE Eqsi/kn/BveKm1Ki+VCPuJC9o3FLV2RmIFuYlY7T/fXRuLGub3g4tp7AU71Rs+GIOp90 CChCD1v+yULQW48stxiJGjUuRccWW6XgMdKzZIV542UWLb8T5u4y7q7XGMsIG7Jb38Fp RN9gmgpFi7qe5KmaL47xh2qdQTNs5zVO9oYzNDlqnU4yus4jKFRXzl6xMx6NDjJmVJcf bz+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=S7wslpAiCNAIoynt+t12sCGV+5ndiJBrxNLdKN+Du8I=; b=nQH0JKJTs7Wm9trLwhfWN6yevpDk6DpOe+ttQXtOrc63hi9B1ceijOMQeWSu13GBH2 TRUHm5+S1cqqVsCYzVyOZe69OXfUhuDOi9i/XVJqtOTmBbKhX3LsKe95c348xArc8JDJ 15QAyTMdrXoKniaKxCLct57v4pPF/oyyvB2M6fInQ5vNsbLrOkBgBBB4EqATyDtjIqpj 2pI56YLYc9/dq0OZp1WWi02PVBlzRHs8yfUnNg8LZRpeQ25osDkSpQx3rQfdJGQYIuqn 1qV6AijOhthWFRDDispJ3WiPEx9krFZMRdoCpK6hVe40iCslxOnYtXKEUZEIJkW+9Zx9 Ldtg== X-Gm-Message-State: AElRT7EHeY2mOUI5WHJYH0t4/mhf8r8QxgNQDEm7CF7OHvIbzQVLu49x 7TF5VczULP+XTgTA5A3DXCpYTt4W X-Google-Smtp-Source: AG47ELt2sKGbDKgo/zZiLN0JXyIDmzUB81I+303/lwKhKKuiOOayeyx0nUDaAJlxc8WdGZuspOadRQ== X-Received: by 10.28.191.12 with SMTP id p12mr4189868wmf.99.1521797497194; Fri, 23 Mar 2018 02:31:37 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id r13sm2036977wre.63.2018.03.23.02.31.36 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 23 Mar 2018 02:31:36 -0700 (PDT) Date: Fri, 23 Mar 2018 10:31:22 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Neil Horman Cc: "Wiles, Keith" , "dev@dpdk.org" Message-ID: <20180323093122.erjd7fxfj4locvt2@bidouze.vm.6wind.com> References: <20180322141037.GC6272@hmswarspite.think-freely.org> <20180322162751.z2kjkpjcg6aw76kk@bidouze.vm.6wind.com> <20180323005349.GC16562@neilslaptop.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180323005349.GC16562@neilslaptop.think-freely.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v2 04/18] eal: add lightweight kvarg parsing utility X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Mar 2018 09:31:38 -0000 On Thu, Mar 22, 2018 at 08:53:49PM -0400, Neil Horman wrote: > On Thu, Mar 22, 2018 at 05:27:51PM +0100, Gaëtan Rivet wrote: > > On Thu, Mar 22, 2018 at 10:10:37AM -0400, Neil Horman wrote: > > > On Wed, Mar 21, 2018 at 05:32:24PM +0000, Wiles, Keith wrote: > > > > > > > > > > > > > On Mar 21, 2018, at 12:15 PM, Gaetan Rivet wrote: > > > > > > > > > > This library offers a quick way to parse parameters passed with a > > > > > key=value syntax. > > > > > > > > > > A single function is needed and finds the relevant element within the > > > > > text. No dynamic allocation is performed. It is possible to chain the > > > > > parsing of each pairs for quickly scanning a list. > > > > > > > > > > This utility is private to the EAL and should allow avoiding having to > > > > > move around the more complete librte_kvargs. > > > > > > > > What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what? > > > > > > > > My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-) > > > > > > > +1, this really doesn't make much sense to me. Two parsing routines seems like > > > its just asking for us to have to fix parsing bugs in two places. If allocation > > > is a concern, I don't see why you can't just change the malloc in > > > rte_kvargs_parse to an automatic allocation on the stack, or a preallocation set > > > of kvargs that can be shared from init time. > > > > I think the existing allocation scheme is fine for other usages (in > > drivers and so on). Not for what I wanted to do. > > > Ok, but thats an adressable issue. you can bifurcate the parse function to an > internal function that accepts any preallocated kvargs struct, and export two > wrapper functions, one which allocates the struct from the heap, another which > allocated automatically on the stack. > Sure, everything is possible. > > > librte_kvargs isn't necessecarily > > > the best parsing library ever, but its not bad, and it just seems wrong to go > > > re-inventing the wheel. > > > > > > > It serves a different purpose than the one I'm pursuing. > > > > This helper is lightweight and private. If I wanted to integrate my > > needs with librte_kvargs, I would be adding new functionalities, making > > it more complex, and for a use-case that is useless for the vast > > majority of users of the lib. > > > Ok, to that end: > > 1) Privacy is not an issue (at least from my understanding of what your doing). > If we start with the assumption that librte_kvargs is capable of satisfying your > needs (even if its not done in an optimal way), the fact that your version of > the function is internal to the library doesn't seem overly relevant, unless > theres something critical to that privacy that I'm missing. > Privacy is only a point I brought up to say that the impact of this function is minimal. People looking to parse their kvargs should not have any ambiguity regarding how they should do so. Only librte_kvargs is available. > 2) Lightweight function seems like something that can be integrated with > librte_kvargs. Looking at it, what may I ask in librte_kvargs is insufficiently > non-performant for your needs, specifically? We talked about the heap > allocation above, is there something else? The string duplication perhaps? > > Mostly the way to use it. The filter strings are bus=value,.../class=value,... where either bus= list or class= list can be omitted, but at least one must appear. I want to read a single kvarg. I do not want to parse the whole string. the '/' signifies the end of the current layer. librte_kvargs does not care about those points. I cannot ask it to only read either bus or class, as it would then throw an error for all the other keys (which the EAL has necessarily no knowledge of). So I would need to: * Add a custom storage scheme * Add a custom parsing mode stopping at the first kvarg * Add an edge-case to ignore the '/', so as not to throw off the rest of the parsing (least it be considered part of the previous kvarg value field). Seeing this, does adding those really specifics functionality help librte_kvargs to be more useful and usable? I do not think so. It would only serve to disrupt the library for a marginal use-case, with the introduction of edge-cases that will blur the specs of the lib's API, making it harder to avoid subtle bugs. Only way to do so sanely would be to add rte_parse_kv as part of librte_kvargs, as is. But then the whole thing does not make sense IMO: no one would care to use it, the maintainance effort is the same, the likelyhood of bugs as well (but in the process we would disrupt the distribution of librte_kvargs by moving it within the EAL). I see no benefit to either solution. > > If that's really an issue, I'm better off simply removing rte_parse_kv > > and writing the parsing by hand within my function. This would be ugly > > and tedious, but less than moving librte_kvargs within EAL and changing > > it to my needs. > I don't think thats necessecary, I just think if you can ennumerate the items > that are non-performant for your needs we can make some changes to librte_kvargs > to optimize around them, or offer parsing options to avoid them, and in the > process avoid some code duplication > I think it makes sense to have specialized functions for specialized use-cases, and forcing the code to be generic and work with all of them will make it more complicated. The genericity would only be worth it if people actually needed to parse the device strings the same way I do. No one has any business doing so. This genericity adds complexity and issues, without even being useful in the first place. -- Gaëtan Rivet 6WIND