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

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

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