[dpdk-dev] [PATCH v2 04/18] eal: add lightweight kvarg parsing utility

[Neil Horman]

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 <gaetan.rivet at 6wind.com> 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.

> >                                               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.

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?


> 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

Neil

> 
> -- 
> Gaëtan Rivet
> 6WIND
>