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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 21 Feb 2017, Jan Beulich wrote:
> >>> 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.

OK


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

Ah, right


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

All right, I'll make the change


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

OK


> >> > +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?
 
Sure

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