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

[Andrew Bennieston <andrew.bennieston@citrix.com>]

Subject:   Re: [PATCH RFC 1/4] xen-netback: Factor queue-specific data
into queue struct.  To:        Paul Durrant
<Paul.Durrant@citrix.com>,"xen-devel@lists.xenproject.org"
<xen-devel@lists.xenproject.org> Cc:        Ian Campbell
<Ian.Campbell@citrix.com>,Wei Liu <wei.liu2@citrix.com> Bcc:
-=-=-=-=-=-=-=-=-=# Don't remove this line #=-=-=-=-=-=-=-=-=- On
16/01/14 11:03, Paul Durrant wrote:
>> -----Original Message----- From: Andrew Bennieston
>> [mailto:andrew.bennieston@citrix.com] Sent: 16 January 2014 10:39 To:
>> Paul Durrant; xen-devel@lists.xenproject.org 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@citrix.com] Sent: 15 January 2014 16:23
>>>> To: xen-devel@lists.xenproject.org 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@citrix.com>
>>>>
>>>> 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@citrix.com>
>>>> --- 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.
Good point. I'll do per-queue stats in V2.

Andrew