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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

> +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. 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)?

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

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

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

> +#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).

Jan

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