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

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



On Tue, 4 Apr 2017, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 31, 2017 at 12:15:16PM -0700, Stefano Stabellini wrote:
> > This patch introduces macros, structs and functions to handle rings in
> > the format described by docs/misc/pvcalls.markdown and
> > docs/misc/9pfs.markdown. The index page (struct __name##_data_intf)
> > contains the indexes and the grant refs to setup two rings.
> > 
> >                Indexes page
> >                +----------------------+
> >                |@0 $NAME_data_intf:   |
> >                |@76: ring_order = 1   |
> >                |@80: ref[0]+          |
> >                |@84: ref[1]+          |
> >                |           |          |
> >                |           |          |
> >                +----------------------+
> >                            |
> >                            v (data ring)
> >                    +-------+-----------+
> >                    |  @0->4095: in     |
> >                    |  ref[0]           |
> >                    |-------------------|
> >                    |  @4096->8191: out |
> >                    |  ref[1]           |
> >                    +-------------------+
> > 
> > $NAME_read_packet and $NAME_write_packet are provided to read or write
> > any data struct from/to the ring. In pvcalls, they are unused. In xen
> > 9pfs, they are used to read or write the 9pfs header. In other protocols
> > they could be used to read/write the whole request structure. See
> > docs/misc/9pfs.markdown:Ring Usage to learn how to check how much data
> > is on the ring, and how to handle notifications.
> > 
> > There is a ring_size parameter to most functions so that protocols using
> > these macros don't have to have a statically defined ring order at build
> > time. In pvcalls for example, each new ring could have a different
> > order.
> > 
> > These macros don't help you share the indexes page or the event channels
> > needed for notifications. You can do that with other out of band
> > mechanisms, such as xenstore or another ring.
> > 
> > It is not possible to use a macro to define another macro with a
> > variable name. For this reason, this patch introduces static inline
> > functions instead, that are not C89 compliant. Additionally, the macro
> > defines a struct with a variable sized array, which is also not C89
> > compliant.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@xxxxxxxxxxx>
> > CC: konrad.wilk@xxxxxxxxxx
> > CC: JBeulich@xxxxxxxx
> > ---
> >  xen/include/public/io/ring.h | 151 
> > +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 151 insertions(+)
> > 
> > diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
> > index 801c0da..7b7794c 100644
> > --- a/xen/include/public/io/ring.h
> > +++ b/xen/include/public/io/ring.h
> > @@ -27,6 +27,21 @@
> >  #ifndef __XEN_PUBLIC_IO_RING_H__
> >  #define __XEN_PUBLIC_IO_RING_H__
> >  
> > +/*
> > + * When #include'ing this header, you need to provide the following
> > + * declaration upfront:
> > + * - standard integers types (uint8_t, uint16_t, etc)
> > + * They are provided by stdint.h of the standard headers.
> > + *
> > + * In addition, if you intend to use the FLEX macros, you also need to
> > + * provide the following, before invoking the FLEX macros:
> > + * - size_t
> > + * - memcpy
> > + * - grant_ref_t
> > + * These declarations are provided by string.h of the standard headers,
> > + * and grant_table.h from the Xen public headers.
> > + */
> > +
> >  #include "../xen-compat.h"
> >  
> >  #if __XEN_INTERFACE_VERSION__ < 0x00030208
> > @@ -313,6 +328,142 @@ typedef struct __name##_back_ring __name##_back_ring_t
> >      (_work_to_do) = RING_HAS_UNCONSUMED_RESPONSES(_r);                  \
> >  } while (0)
> >  
> > +
> > +/*
> > + * DEFINE_XEN_FLEX_RING_AND_INTF defines two monodirectional rings and
> > + * functions to check if there is data on the ring, and to read and
> > + * write to them.
> > + *
> > + * DEFINE_XEN_FLEX_RING is similar to DEFINE_XEN_FLEX_RING_AND_INTF, but
> > + * does not define the indexes page. As different protocols can have
> > + * extensions to the basic format, this macro allow them to define their
> > + * own struct.
> > + *
> > + * XEN_FLEX_RING_SIZE
> > + *   Convenience macro to calculate the size of one of the two rings
> > + *   from the overall order.
> > + *
> > + * $NAME_mask
> > + *   Function to apply the size mask to an index, to reduce the index
> > + *   within the range [0-size].
> > + *
> > + * $NAME_read_packet
> > + *   Function to read data from the ring. The amount of data to read is
> > + *   specified by the "size" argument.
> > + *
> > + * $NAME_write_packet
> > + *   Function to write data to the ring. The amount of data to write is
> > + *   specified by the "size" argument.
> > + *
> > + * $NAME_get_ring_ptr
> > + *   Convenience function that returns a pointer to read/write to the
> > + *   ring at the right location.
> > + *
> > + * $NAME_data_intf
> > + *   Indexes page, shared between frontend and backend. It also
> > + *   contains the array of grant refs.
> > + *
> > + * $NAME_queued
> > + *   Function to calculate how many bytes are currently on the ring,
> > + *   ready to be read. It can also be used to calculate how much free
> > + *   space is currently on the ring (ring_size - $NAME_queued()).
> 
> s/ring_size/XEN_FLEX_RING_SIZE/ ?

OK, I'll write (XEN_FLEX_RING_SIZE() - $NAME_queued()).
I think that also adding all the parameters would be too wordy.


> > + */
> > +
> > +#define XEN_PAGE_SHIFT 12
> 
> Perhaps a comment explaining why you have that? It looks very architecture
> dependent?
>
> Or perhaps you could do with
> 
> #ifndef XEN_PAGE_SHIFT
> /*
>  * Hypercall interface only supports only 4Kb page unless the architecture
>  * overrides it.
>  */

I don't think there are any architecture overrides actually. On ARM is
always defined as 4K, no matter the choice of page granularity made by
the operating system.


> #define XEN_PAGE_SHIFT 12
> #endif
> 
> ?

I'll add the #ifndef to avoid possible collisions and add this comment:

  The PAGE_SIZE (thus, also PAGE_SHIFT) for ring protocols and hypercall
  interfaces is always 4K, regardless of the architecture and page
  granularity chosen by operating systems.


> > +#define XEN_FLEX_RING_SIZE(order)                                          
> >    \
> > +    (1UL << ((order) + XEN_PAGE_SHIFT - 1))
> > +
> > +#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 unsigned char *name##_get_ring_ptr(unsigned char *buf,       
> >    \
> > +                                                 RING_IDX idx,             
> >    \
> > +                                                 RING_IDX ring_size)       
> >    \
> > +{                                                                          
> >    \
> > +    return buf + name##_mask(idx, ring_size);                              
> >    \
> > +}                                                                          
> >    \
> > +                                                                           
> >    \
> > +static inline void name##_read_packet(void *opaque,                        
> >    \
> > +                                      const unsigned char *buf,            
> >    \
> > +                                      size_t size,                         
> >    \
> > +                                      RING_IDX masked_prod,                
> >    \
> > +                                      RING_IDX *masked_cons,               
> >    \
> > +                                      RING_IDX ring_size)                  
> >    \
> > +{                                                                          
> >    \
> > +    if (*masked_cons < masked_prod ||                                      
> >    \
> > +            size <= ring_size - *masked_cons) {                            
> >    \
> 
> 'size <= ..' is not aligned here while it is in _write_packet?
> 
> Besides that it looks fine to me.

I'll align


> > +        memcpy(opaque, buf + *masked_cons, size);                          
> >    \
> > +    } else {                                                               
> >    \
> > +        memcpy(opaque, buf + *masked_cons, ring_size - *masked_cons);      
> >    \
> > +        memcpy((unsigned char *)opaque + ring_size - *masked_cons, buf,    
> >    \
> > +                size - (ring_size - *masked_cons));                        
> >    \
> > +    }                                                                      
> >    \
> > +    *masked_cons = name##_mask(*masked_cons + size, ring_size);            
> >    \
> > +}                                                                          
> >    \
> > +                                                                           
> >    \
> > +static inline void name##_write_packet(unsigned char *buf,                 
> >    \
> > +                                       const void *opaque,                 
> >    \
> > +                                       size_t size,                        
> >    \
> > +                                       RING_IDX *masked_prod,              
> >    \
> > +                                       RING_IDX masked_cons,               
> >    \
> > +                                       RING_IDX ring_size)                 
> >    \
> > +{                                                                          
> >    \
> > +    if (*masked_prod < masked_cons ||                                      
> >    \
> > +        size <= ring_size - *masked_prod) {                                
> >    \
> > +        memcpy(buf + *masked_prod, opaque, size);                          
> >    \
> > +    } else {                                                               
> >    \
> > +        memcpy(buf + *masked_prod, opaque, ring_size - *masked_prod);      
> >    \
> > +        memcpy(buf, (unsigned char *)opaque + (ring_size - *masked_prod),  
> >    \
> > +                size - (ring_size - *masked_prod));                        
> >    \
> > +    }                                                                      
> >    \
> > +    *masked_prod = name##_mask(*masked_prod + size, ring_size);            
> >    \
> > +}                                                                          
> >    \
> > +                                                                           
> >    \
> > +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;                                                           
> >    \
> > +}                                                                          
> >    \
> > +                                                                           
> >    \
> > +struct name##_data {                                                       
> >    \
> > +    unsigned char *in; /* half of the allocation */                        
> >    \
> > +    unsigned char *out; /* half of the allocation */                       
> >    \
> > +}
> > +
> > +#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)
> > +
> >  #endif /* __XEN_PUBLIC_IO_RING_H__ */
> >  
> >  /*
> > -- 
> > 1.9.1
> > 
> 

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