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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.02.17 at 00:26, <sstabellini@xxxxxxxxxx> wrote:
> On Mon, 20 Feb 2017, Jan Beulich wrote:
>> >>> On 17.02.17 at 23:38, <sstabellini@xxxxxxxxxx> wrote:
>> But the use of inline functions here is questionable
>> in the first place - so far there are none, as they're not standard
>> C89. Granted they are being used in a macro only, so won't cause
>> immediate issues for code not using the macros, but anyway.
> 
> I did it because it is not possible to use a macro to define another
> macro with a variable name. In other words, the following does not work:
> 
>   #define DEFINE_XEN_FLEX_RING(name, packet_t)                              \
>   #define name##_mask(idx, ring_size) (idx & (ring_size - 1))      
> 
> I prefer to drop C89 compliance to keep cleaner code, but if you have
> better suggestions, I would be happy to hear them.

Well, okay then, but please add a respective remark to the commit
message then.

>> > +static inline void __name##_read_packet(char *buf,                        
>> >     \
>> 
>> const
> 
> We cannot use const char *buf, because we are about to pass buf to
> memcpy.

I don't understand: memcpy()'s second parameter is const.

>> > +        RING_IDX *masked_prod, RING_IDX *masked_cons,                     
>> >     \
>> 
>> You don't update *masked_prod - why do you need a pointer here?
> 
> When using this functions in pvcalls and xen-9pfs, I found that it was
> less confusing for me if the two index parameters had the same type. In
> fact, I started with what you suggested, then changed it back to this.

I don't find this a convincing argument. The parameters will differ
in constness anyway due to the different transfer direction, and
using an extra layer of indirection just for aesthetic reasons seems
at least odd to me. 

>> > +struct __name##_data {                                                    
>> >     \
>> > +    char *in; /* half of the allocation */                                
>> >     \
>> > +    char *out; /* half of the allocation */                               
>> >     \
>> 
>> Why plain char? Void would seem the most neutral option here, but
>> if arithmetic needs doing inside the header, then unsigned char may
>> need to be used.
> 
> The types here should match the type of the "buf" parameter used in the
> read/write_packet functions. We cannot use void as we do arithmetic. If
> you think unsigned char is more appropriate, I'll also change the type
> of the buf arguments elsewhere.

Yes please - plain char is mainly meant for use with strings, not
arbitrary binary data.

>> > +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[];                                                    
>> >     \
>> > +};                                                                        
>> >     \
>> 
>> Above you talk about the option of defining private _data_intf
>> structures. How is that supposed to work when you define a
>> suitably named structure already here? Other than #define-s
>> one can't undo such a type definition.
> 
> Yes, but it is always possible to use a different name. For example,
> this is what I did in the pvcalls header:
> 
>   struct __pvcalls_data_intf {
>     RING_IDX in_cons, in_prod, in_error;
> 
>     uint8_t pad1[52];
> 
>     RING_IDX out_cons, out_prod, out_error;
> 
>     uint8_t pad2[52];
> 
>     RING_IDX ring_order;
>     grant_ref_t ref[];
>   };
> 
> This struct is only here as a reference, we don't have to actually
> define it in ring.h. If you think it's best, I could introduce it only
> as a comment.

How about having two variants of the macro - one (the base)
version without this, and a second invoking the first and adding
this?

Jan

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