[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 1/4] ring.h: introduce macros to handle monodirectional rings with multiple req sizes



On Wed, 29 Mar 2017, Jan Beulich wrote:
> >>> On 29.03.17 at 00:08, <sstabellini@xxxxxxxxxx> wrote:
> > +#define DEFINE_XEN_FLEX_RING(name)                                         
> >    \
> > +static inline RING_IDX name##_mask(RING_IDX idx, RING_IDX ring_size)       
> >    \
> > +{                                                                          
> >    \
> > +    return (idx & (ring_size - 1));                                        
> >    \
> > +}                                                                          
> >    \
> > +                                                                           
> >    \
> > +static inline RING_IDX name##_mask_order(RING_IDX idx, RING_IDX 
> > ring_order)   \
> > +{                                                                          
> >    \
> > +    return (idx & (XEN_FLEX_RING_SIZE(ring_order) - 1));                   
> >    \
> > +}                                                                          
> >    \
> 
> Do you really need both (and if you do, perhaps the latter should
> call the former)? I also find the mixture of ring_order and ring_size
> parameters of later functions a little strange.

Actually, I don't need two, I'll drop name##_mask_order. I'll change
the parameter below to be ring_size for consistency.


> > +static inline unsigned char *name##_get_ring_ptr(unsigned char *buf,       
> >    \
> > +                                                 RING_IDX idx,             
> >    \
> > +                                                 RING_IDX ring_order)      
> >    \
> > +{                                                                          
> >    \
> > +    return buf + name##_mask_order(idx, ring_order);                       
> >    \
> 
> Please be consistent with parenthesizing the operand of return:
> The earlier two functions have an unnecessary pair of parens,
> so personally I'd prefer those to be dropped. But if you prefer to
> have them, add them everywhere.

OK


> > +static inline void name##_read_packet(const unsigned char *buf,            
> >    \
> > +        RING_IDX masked_prod, RING_IDX *masked_cons,                       
> >    \
> > +        RING_IDX ring_size, void *opaque, size_t size)                     
> >    \
> 
> Especially with so many parameters I think some extra thought
> should be spent on their ordering: Primarily this is a memcpy()-
> like function, so I would kind of expect destination description,
> source description (each of which may require more than one
> parameter), size, auxiliary.

I'll reorder the parameters.


> As to the auxiliary part (especially
> ring_size) - there's no structure you could pass a pointer to,
> taking care of more than one of these, is there (struct
> name##_data_intf would at least appear to be a candidate,
> but is not always available)?

That is the problem, it is not always available. I prefer to keep them
separate.


> I'm also not really clear whether it wouldn't be better for both
> input and output to be void * (input remaining const of course).

I think it's a matter of taste: source is unsigned char* because it is
of the same type as the underlying ring buffer to read data from. I'll
leave it as is for now.


> And finally please indent function parameter declarations
> uniformly throughout the patch. If you prefer to follow the style
> of the declaration right above, then please reduce indentation 
> to four spaces (to match that of function scope local variable
> declarations).

I'll fix indentation.


> > +static inline RING_IDX name##_queued(RING_IDX prod,                        
> >    \
> > +        RING_IDX cons, RING_IDX ring_size)                                 
> >    \
> > +{                                                                          
> >    \
> > +    RING_IDX size;                                                         
> >    \
> > +                                                                           
> >    \
> > +    if (prod == cons)                                                      
> >    \
> > +        return 0;                                                          
> >    \
> > +                                                                           
> >    \
> > +    prod = name##_mask(prod, ring_size);                                   
> >    \
> > +    cons = name##_mask(cons, ring_size);                                   
> >    \
> > +                                                                           
> >    \
> > +    if (prod == cons)                                                      
> >    \
> > +        return ring_size;                                                  
> >    \
> > +                                                                           
> >    \
> > +    if (prod > cons)                                                       
> >    \
> > +        size = prod - cons;                                                
> >    \
> > +    else                                                                   
> >    \
> > +        size = ring_size - (cons - prod);                                  
> >    \
> > +    return size;                                                           
> >    \
> > +};
> 
> Stray semicolon at end of function definition.

OK


> > +#define DEFINE_XEN_FLEX_RING_AND_INTF(name)                                
> >    \
> > +struct name##_data_intf {                                                  
> >    \
> > +    RING_IDX in_cons, in_prod;                                             
> >    \
> > +                                                                           
> >    \
> > +    uint8_t pad1[56];                                                      
> >    \
> > +                                                                           
> >    \
> > +    RING_IDX out_cons, out_prod;                                           
> >    \
> > +                                                                           
> >    \
> > +    uint8_t pad2[56];                                                      
> >    \
> > +                                                                           
> >    \
> > +    RING_IDX ring_order;                                                   
> >    \
> > +    grant_ref_t ref[];                                                     
> >    \
> > +};                                                                         
> >    \
> > +DEFINE_XEN_FLEX_RING(name);
> 
> The trailing semicolon here should be left out, requiring the use site
> to put one after the macro invocation. Some compilers warn about
> such stray semicolons. This implies that the last element of 
> DEFINE_XEN_FLEX_RING() should also not be an inline function
> definition (as a semicolon placed after the macro invocation would
> then also possibly trigger a compiler warning).

OK

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.