From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Bennieston Subject: Re: [PATCH RFC 3/4] xen-netfront: Factor queue-specific data into queue struct. Date: Thu, 16 Jan 2014 10:08:43 +0000 Message-ID: <52D7AFAB.50404@citrix.com> References: <1389803004-31812-1-git-send-email-andrew.bennieston@citrix.com> <1389803004-31812-4-git-send-email-andrew.bennieston@citrix.com> <20140116002500.GI5331@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1W3jsW-00084I-Kx for xen-devel@lists.xenproject.org; Thu, 16 Jan 2014 10:08:48 +0000 In-Reply-To: <20140116002500.GI5331@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: xen-devel@lists.xenproject.org, paul.durrant@citrix.com, ian.campbell@citrix.com List-Id: xen-devel@lists.xenproject.org On 16/01/14 00:25, Wei Liu wrote: > On Wed, Jan 15, 2014 at 04:23:23PM +0000, Andrew J. Bennieston wrote: > [...] >> + >> +struct netfront_queue { >> + unsigned int number; /* Queue number, 0-based */ >> + char name[IFNAMSIZ+4]; /* DEVNAME-qN */ >> + struct netfront_info *info; >> >> struct napi_struct napi; >> >> @@ -93,10 +96,8 @@ struct netfront_info { >> unsigned int tx_evtchn, rx_evtchn; >> unsigned int tx_irq, rx_irq; >> /* Only used when split event channels support is enabled */ >> - char tx_irq_name[IFNAMSIZ+4]; /* DEVNAME-tx */ >> - char rx_irq_name[IFNAMSIZ+4]; /* DEVNAME-rx */ >> - >> - struct xenbus_device *xbdev; >> + char tx_irq_name[IFNAMSIZ+7]; /* DEVNAME-qN-tx */ >> + char rx_irq_name[IFNAMSIZ+7]; /* DEVNAME-qN-rx */ > > Basically you're anticipating the maximum number of queues is below 10 > here. I think leaving one more byte here won't hurt, just in case you > will have more than 10 queues. The same goes to netback as well. > > In your next patch you have max_queue as 16 by default, which has > already broken your assumption here already. :-) > Yes, this should be fixed. I wouldn't expect more than 100 queues, so adding another byte here should be sufficient. In any case, I don't think the names are used for anything other than human readable output, so there won't be any functional impact to truncating them. >> >> spinlock_t tx_lock; >> struct xen_netif_tx_front_ring tx; >> @@ -139,6 +140,17 @@ struct netfront_info { >> unsigned long rx_pfn_array[NET_RX_RING_SIZE]; >> struct multicall_entry rx_mcl[NET_RX_RING_SIZE+1]; >> struct mmu_update rx_mmu[NET_RX_RING_SIZE]; >> +}; >> + > [...] >> static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) >> { >> unsigned short id; >> @@ -555,6 +575,15 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) >> unsigned int offset = offset_in_page(data); >> unsigned int len = skb_headlen(skb); >> unsigned long flags; >> + struct netfront_queue *queue = NULL; >> + u16 queue_index; >> + >> + /* Drop the packet if no queues are set up */ >> + if (np->num_queues < 1 || np->queues == NULL) > > Same as the comment in netback, you won't need both. Agreed. I will switch to if (np->num_queues < 1) for the same reason I gave in the netback mail. Andrew. > > And the rest of the patch is basically replacing np with queue and > putting things in loops. So I stopped here... > > Wei. >