[PATCH] um: Migrate vector drivers to NAPI

[Anton Ivanov]

On 21/01/2022 09:29, Johannes Berg wrote:
> On Fri, 2022-01-21 at 08:55 +0000, anton.ivanov at cambridgegreys.com
> wrote:
>> From: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>>
>> Migrate UML vector drivers from a bespoke scheduling mechanism
>> to NAPI.
> 
> Nice :)
> 
>> -static void vector_rx(struct vector_private *vp)
>> -{
>> -	int err;
>> -	int iter = 0;
>> -
>> -	if ((vp->options & VECTOR_RX) > 0)
>> -		while (((err = vector_mmsg_rx(vp)) > 0) && (iter < MAX_ITERATIONS))
> 
> You can remove the MAX_ITERATIONS define now.

Thanks, missed that.

> 
> 
>> +static int vector_poll(struct napi_struct *napi, int budget)
>>   {
>> -	struct vector_private *vp = from_tasklet(vp, t, tx_poll);
>> +	struct vector_private *vp = container_of(napi, struct vector_private, napi);
>> +	int work_done = 0;
>> +	int err;
>> +	bool tx_enqueued = false;
>>   
>> -	vp->estats.tx_kicks++;
>> -	vector_send(vp->tx_queue);
>> +	if ((vp->options & VECTOR_TX) != 0)
>> +		tx_enqueued = (vector_send(vp->tx_queue) > 0);
>> +	if ((vp->options & VECTOR_RX) > 0)
>> +		err = vector_mmsg_rx(vp);
>> +	else {
>> +		err = vector_legacy_rx(vp);
> 
> It feels like you should honour the NAPI and pass the budget down, using
> it to limit the number of packets processed for TX/RX, something like
> 
> tx_enqueued = vector_send(..., budget);
> budget -= tx_enqueued;

Good idea :)

> 
> if (budget > 0)
> 	err = vector_mmsg_rx(vp, budget);
> 
> etc.
> 
>> @@ -1264,6 +1251,10 @@ static int vector_net_open(struct net_device *dev)
>>   		if (vp->header_txbuffer == NULL)
>>   			goto out_close;
>>   	}
>> +	/* NAPI */
>> +
> 
> That comment feels a bit pointless, but whatever :)

Thanks, I used that to make my life easier editing it. Forgot to remove it.

> 
>> +	netif_napi_add(vp->dev, &vp->napi, vector_poll, 64);
>> +	napi_enable(&vp->napi);
> 
> I think this should only be done once when you create the netdev?

I need to have a look around the kernel and think on this one. Historically, UML does a few things in open() which are closer to device instantiation for real hardware.

> 
> 
>> @@ -1312,9 +1303,11 @@ static int vector_net_open(struct net_device *dev)
>>   	 * SIGIOs never arrive, and the net never works.
>>   	 */
>>   
>> -	vector_rx(vp);
>>   
>>   	vector_reset_stats(vp);
>> +
>> +	napi_schedule(&vp->napi);
>>
> 
> The comment should probably move with this?

Yes. Well spotted.

> 
> johannes
> 

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/