From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Bennieston Subject: Re: [PATCH RFC 1/4] xen-netback: Factor queue-specific data into queue struct. Date: Thu, 16 Jan 2014 11:06:41 +0000 Message-ID: <52D7BD41.70408@citrix.com> References: <1389803004-31812-1-git-send-email-andrew.bennieston@citrix.com> <1389803004-31812-2-git-send-email-andrew.bennieston@citrix.com> <9AAE0902D5BC7E449B7C8E4E778ABCD0208DDA@AMSPEX01CL01.citrite.net> <52D7B6B3.5060303@citrix.com> <9AAE0902D5BC7E449B7C8E4E778ABCD0208FAE@AMSPEX01CL01.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1W3kmb-0005El-FE for xen-devel@lists.xenproject.org; Thu, 16 Jan 2014 11:06:45 +0000 In-Reply-To: <9AAE0902D5BC7E449B7C8E4E778ABCD0208FAE@AMSPEX01CL01.citrite.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Paul Durrant , "xen-devel@lists.xenproject.org" Cc: Wei Liu , Ian Campbell List-Id: xen-devel@lists.xenproject.org Subject: Re: [PATCH RFC 1/4] xen-netback: Factor queue-specific data into queue struct. To: Paul Durrant ,"xen-devel@lists.xenproject.org" Cc: Ian Campbell ,Wei Liu 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" >>>> >>>> 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 >>>> --- 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