Re: [Xen-devel] [PATCH RFC 1/4] xen-netback: Factor queue-specific data into queue struct.

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Bennieston [mailto:andrew.bennieston@xxxxxxxxxx]
> Sent: 16 January 2014 10:39
> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Ian Campbell; Wei Liu
> Subject: Re: [PATCH RFC 1/4] xen-netback: Factor queue-specific data into
> queue struct.
> 
> On 16/01/14 10:23, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew J. Bennieston [mailto:andrew.bennieston@xxxxxxxxxx]
> >> Sent: 15 January 2014 16:23
> >> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Ian Campbell; Wei Liu; Paul Durrant; Andrew Bennieston
> >> Subject: [PATCH RFC 1/4] xen-netback: Factor queue-specific data into
> >> queue struct.
> >>
> >> From: "Andrew J. Bennieston" <andrew.bennieston@xxxxxxxxxx>
> >>
> >> In preparation for multi-queue support in xen-netback, move the
> >> queue-specific data from struct xenvif into struct xenvif_queue, and
> >> update the rest of the code to use this.
> >>
> >> Also adds loops over queues where appropriate, even though only one is
> >> configured at this point, and uses alloc_netdev_mq() and the
> >> corresponding multi-queue netif wake/start/stop functions in preparation
> >> for multiple active queues.
> >>
> >> Finally, implements a trivial queue selection function suitable for
> >> ndo_select_queue, which simply returns 0 for a single queue and uses
> >> skb_get_rxhash() to compute the queue index otherwise.
> >>
> >> Signed-off-by: Andrew J. Bennieston <andrew.bennieston@xxxxxxxxxx>
> >> ---
> >>   drivers/net/xen-netback/common.h    |   66 +++--
> >>   drivers/net/xen-netback/interface.c |  308 +++++++++++++--------
> >>   drivers/net/xen-netback/netback.c   |  516 +++++++++++++++++---------
> ----
> >> -----
> >>   drivers/net/xen-netback/xenbus.c    |   89 ++++--
> >>   4 files changed, 556 insertions(+), 423 deletions(-)
> >>
> >> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-
> >> netback/common.h
> >> index c47794b..54d2eeb 100644
> >> --- a/drivers/net/xen-netback/common.h
> >> +++ b/drivers/net/xen-netback/common.h
> >> @@ -108,17 +108,19 @@ struct xenvif_rx_meta {
> >>    */
> >>   #define MAX_GRANT_COPY_OPS (MAX_SKB_FRAGS *
> >> XEN_NETIF_RX_RING_SIZE)
> >>
> >> -struct xenvif {
> >> -  /* Unique identifier for this interface. */
> >> -  domid_t          domid;
> >> -  unsigned int     handle;
> >> +struct xenvif;
> >> +
> >> +struct xenvif_queue { /* Per-queue data for xenvif */
> >> +  unsigned int number; /* Queue number, 0-based */
> >> +  char name[IFNAMSIZ+4]; /* DEVNAME-qN */
> >
> > I wonder whether it would be neater to #define the name size here...
> >
> 
> Absolutely. I'll do this in V2.
> 
> >> +  struct xenvif *vif; /* Parent VIF */
> >>
> >>    /* Use NAPI for guest TX */
> >>    struct napi_struct napi;
> >>    /* When feature-split-event-channels = 0, tx_irq = rx_irq. */
> >>    unsigned int tx_irq;
> >>    /* Only used when feature-split-event-channels = 1 */
> >> -  char tx_irq_name[IFNAMSIZ+4]; /* DEVNAME-tx */
> >> +  char tx_irq_name[IFNAMSIZ+7]; /* DEVNAME-qN-tx */
> >
> > ...and the IRQ name size here. It's kind of ugly to have + some_magic_value
> in array definitions.
> >
> 
> As above.
> 
> >>    struct xen_netif_tx_back_ring tx;
> >>    struct sk_buff_head tx_queue;
> >>    struct page *mmap_pages[MAX_PENDING_REQS];
> >> @@ -140,7 +142,7 @@ struct xenvif {
> >>    /* When feature-split-event-channels = 0, tx_irq = rx_irq. */
> >>    unsigned int rx_irq;
> >>    /* Only used when feature-split-event-channels = 1 */
> >> -  char rx_irq_name[IFNAMSIZ+4]; /* DEVNAME-rx */
> >> +  char rx_irq_name[IFNAMSIZ+7]; /* DEVNAME-qN-rx */
> >>    struct xen_netif_rx_back_ring rx;
> >>    struct sk_buff_head rx_queue;
> >>
> >> @@ -150,14 +152,27 @@ struct xenvif {
> >>     */
> >>    RING_IDX rx_req_cons_peek;
> >>
> >> -  /* This array is allocated seperately as it is large */
> >> -  struct gnttab_copy *grant_copy_op;
> >> +  struct gnttab_copy grant_copy_op[MAX_GRANT_COPY_OPS];
> >
> > I see you brought this back in line, which is reasonable as the queue is now
> a separately allocated struct.
> >
> 
> Indeed; trying to keep the number of separate allocs/frees to a minimum,
> for everybody's sanity!
> 
> >>
> >>    /* We create one meta structure per ring request we consume, so
> >>     * the maximum number is the same as the ring size.
> >>     */
> >>    struct xenvif_rx_meta meta[XEN_NETIF_RX_RING_SIZE];
> >>
> >> +  /* Transmit shaping: allow 'credit_bytes' every 'credit_usec'. */
> >> +  unsigned long   credit_bytes;
> >> +  unsigned long   credit_usec;
> >> +  unsigned long   remaining_credit;
> >> +  struct timer_list credit_timeout;
> >> +  u64 credit_window_start;
> >> +
> >> +};
> >> +
> >> +struct xenvif {
> >> +  /* Unique identifier for this interface. */
> >> +  domid_t          domid;
> >> +  unsigned int     handle;
> >> +
> >>    u8               fe_dev_addr[6];
> >>
> >>    /* Frontend feature information. */
> >> @@ -171,12 +186,9 @@ struct xenvif {
> >>    /* Internal feature information. */
> >>    u8 can_queue:1;     /* can queue packets for receiver? */
> >>
> >> -  /* Transmit shaping: allow 'credit_bytes' every 'credit_usec'. */
> >> -  unsigned long   credit_bytes;
> >> -  unsigned long   credit_usec;
> >> -  unsigned long   remaining_credit;
> >> -  struct timer_list credit_timeout;
> >> -  u64 credit_window_start;
> >> +  /* Queues */
> >> +  unsigned int num_queues;
> >> +  struct xenvif_queue *queues;
> >>
> >>    /* Statistics */
> >
> > Do stats need to be per-queue (and then possibly aggregated at query
> time)?
> >
> 
> Aside from the potential to see the stats for each queue, which may be
> useful in some limited circumstances for performance testing or
> debugging, I don't see what this buys us...
> 

Well, if you have multiple queues running simultaneously, do you make sure 
global stats are atomically adjusted? I didn't see any code to that effect, and 
since atomic ops are expensive it's usually better to keep per queue stats and 
aggregate at the point of query.

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