[dpdk-dev] [dpdk-dev,v5,1/3] mempool: support external handler

Jan Viktorin viktorin at rehivetech.com
Wed Jun 1 14:30:18 CEST 2016


On Tue, 31 May 2016 22:40:59 +0200
Olivier MATZ <olivier.matz at 6wind.com> wrote:

> Hi,
> 
> On 05/31/2016 03:47 PM, Hunt, David wrote:
> > On 5/31/2016 1:06 PM, Jan Viktorin wrote:  
> >> On Tue, 31 May 2016 10:09:42 +0100
> >> "Hunt, David" <david.hunt at intel.com> wrote:
> >>  
> >>> The *p pointer is the opaque data for a given mempool handler (ring,
> >>> array, linked list, etc)  
> >> Again, doc comments...
> >>
> >> I don't like the obj_table representation to be an array of void *. I
> >> could see
> >> it already in DPDK for defining Ethernet driver queues, so, it's
> >> probably not
> >> an issue. I just say, I would prefer some basic type safety like
> >>
> >> struct rte_mempool_obj {
> >>     void *p;
> >> };
> >>
> >> Is there somebody with different opinions?
> >>
> >> [...]  
> > 
> > Comments added. I've left as a void* for the moment.  
> 
> Jan, could you please detail why you think having a
> rte_mempool_obj structure brings more safety?

First, void * does not say anything about the purpose of the argument.
So, anybody, who is not familiar with the mempool internals would be
lost for a while (as I was when studying the Ethernet queue API of DPDK).

The type safety (in C, LoL) here is in the means of messing up order of arguments.
When there are more void * args, you can accidently pass something wrong.
DPDK has quite strict compiler settings, however, I don't consider it to be
a good practice in general.

When you have a named struct or a typedef, its definition usually contains doc
comments describing its purposes. Nobody usually writes good doc comments for
void * arguments of functions.

> 
> For now, I'm in favor of keeping the array of void *, because
> that's what we use in other mempool or ring functions.

It was just a suggestion... I don't consider this to be an issue (as stated
earlier).

Jan

> 
> 
> >>>>> +/** Structure defining a mempool handler. */  
> >>>> Later in the text, I suggested to rename rte_mempool_handler to
> >>>> rte_mempool_ops.
> >>>> I believe that it explains the purpose of this struct better. It
> >>>> would improve
> >>>> consistency in function names (the *_ext_* mark is very strange and
> >>>> inconsistent).  
> >>> I agree. I've gone through all the code and renamed to
> >>> rte_mempool_handler_ops.  
> >> Ok. I meant rte_mempool_ops because I find the word "handler" to be
> >> redundant.  
> > 
> > I prefer the use of the word handler, unless others also have opinions
> > either way?  
> 
> Well, I think rte_mempool_ops is clear enough, and shorter,
> so I'd vote for it.
> 
> 
> Regards,
> Olivier



-- 
   Jan Viktorin                  E-mail: Viktorin at RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic


More information about the dev mailing list