Re: [PATCH net-next 11/16] idpf: prepare structures to support XDP

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Fri, 7 Mar 2025 14:27:13 +0100

> On Wed, Mar 05, 2025 at 05:21:27PM +0100, Alexander Lobakin wrote:
>> From: Michal Kubiak <michal.kubiak@intel.com>
>>
>> Extend basic structures of the driver (e.g. 'idpf_vport', 'idpf_*_queue',
>> 'idpf_vport_user_config_data') by adding members necessary to support XDP.
>> Add extra XDP Tx queues needed to support XDP_TX and XDP_REDIRECT actions
>> without interfering with regular Tx traffic.
>> Also add functions dedicated to support XDP initialization for Rx and
>> Tx queues and call those functions from the existing algorithms of
>> queues configuration.

[...]

>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
>> index 59b1a1a09996..1ca322bfe92f 100644
>> --- a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
>> +++ b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
>> @@ -186,9 +186,11 @@ static void idpf_get_channels(struct net_device *netdev,
>>  {
>>  	struct idpf_netdev_priv *np = netdev_priv(netdev);
>>  	struct idpf_vport_config *vport_config;
>> +	const struct idpf_vport *vport;
>>  	u16 num_txq, num_rxq;
>>  	u16 combined;
>>  
>> +	vport = idpf_netdev_to_vport(netdev);
>>  	vport_config = np->adapter->vport_config[np->vport_idx];
>>  
>>  	num_txq = vport_config->user_config.num_req_tx_qs;
>> @@ -202,8 +204,8 @@ static void idpf_get_channels(struct net_device *netdev,
>>  	ch->max_rx = vport_config->max_q.max_rxq;
>>  	ch->max_tx = vport_config->max_q.max_txq;
>>  
>> -	ch->max_other = IDPF_MAX_MBXQ;
>> -	ch->other_count = IDPF_MAX_MBXQ;
>> +	ch->max_other = IDPF_MAX_MBXQ + vport->num_xdp_txq;
>> +	ch->other_count = IDPF_MAX_MBXQ + vport->num_xdp_txq;
> 
> That's new I think. Do you explain somewhere that other `other` will carry
> xdpq count? Otherwise how would I know to interpret this value?

Where? :D

> 
> Also from what I see num_txq carries (txq + xdpq) count. How is that
> affecting the `combined` from ethtool_channels?

No changes in combined/Ethtool, num_txq is not used there. Stuff like
req_txq_num includes skb queues only.

> 
>>  
>>  	ch->combined_count = combined;
>>  	ch->rx_count = num_rxq - combined;
>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
>> index 2594ca38e8ca..0f4edc9cd1ad 100644
> 
> (...)
> 
>> +
>> +/**
>> + * __idpf_xdp_rxq_info_init - Setup XDP RxQ info for a given Rx queue
>> + * @rxq: Rx queue for which the resources are setup
>> + * @arg: flag indicating if the HW works in split queue mode
>> + *
>> + * Return: 0 on success, negative on failure.
>> + */
>> +static int __idpf_xdp_rxq_info_init(struct idpf_rx_queue *rxq, void *arg)
>> +{
>> +	const struct idpf_vport *vport = rxq->q_vector->vport;
>> +	bool split = idpf_is_queue_model_split(vport->rxq_model);
>> +	const struct page_pool *pp;
>> +	int err;
>> +
>> +	err = __xdp_rxq_info_reg(&rxq->xdp_rxq, vport->netdev, rxq->idx,
>> +				 rxq->q_vector->napi.napi_id,
>> +				 rxq->rx_buf_size);
>> +	if (err)
>> +		return err;
>> +
>> +	pp = split ? rxq->bufq_sets[0].bufq.pp : rxq->pp;
>> +	xdp_rxq_info_attach_page_pool(&rxq->xdp_rxq, pp);
>> +
>> +	if (!split)
>> +		return 0;
> 
> why do you care about splitq model if on next patch you don't allow
> XDP_SETUP_PROG for that?

This function is called unconditionally for both queue models. If we
don't account it here, we'd break regular traffic flow.

(singleq will be removed soon, don't take it seriously anyway)

[...]

>> +int idpf_vport_xdpq_get(const struct idpf_vport *vport)
>> +{
>> +	struct libeth_xdpsq_timer **timers __free(kvfree) = NULL;
> 
> please bear with me here - so this array will exist as long as there is a
> single timers[i] allocated? even though it's a local var?

No problem.

No, this array will be freed when the function exits. This array is an
array of pointers to iterate in a loop and assign timers to queues. When
we exit this function, it's no longer needed.
I can't place the whole array on the stack since I don't know the actual
queue count + it can be really big (1024 pointers * 8 = 8 Kb, even 128
or 256 queues is already 1-2 Kb).

The actual timers are allocated separately and NUMA-locally below.

> 
> this way you avoid the need to store it in vport?
> 
>> +	struct net_device *dev;
>> +	u32 sqs;
>> +
>> +	if (!idpf_xdp_is_prog_ena(vport))
>> +		return 0;
>> +
>> +	timers = kvcalloc(vport->num_xdp_txq, sizeof(*timers), GFP_KERNEL);
>> +	if (!timers)
>> +		return -ENOMEM;
>> +
>> +	for (u32 i = 0; i < vport->num_xdp_txq; i++) {
>> +		timers[i] = kzalloc_node(sizeof(*timers[i]), GFP_KERNEL,
>> +					 cpu_to_mem(i));
>> +		if (!timers[i]) {
>> +			for (int j = i - 1; j >= 0; j--)
>> +				kfree(timers[j]);
>> +
>> +			return -ENOMEM;
>> +		}
>> +	}
>> +
>> +	dev = vport->netdev;
>> +	sqs = vport->xdp_txq_offset;
>> +
>> +	for (u32 i = sqs; i < vport->num_txq; i++) {
>> +		struct idpf_tx_queue *xdpq = vport->txqs[i];
>> +
>> +		xdpq->complq = xdpq->txq_grp->complq;
>> +
>> +		idpf_queue_clear(FLOW_SCH_EN, xdpq);
>> +		idpf_queue_clear(FLOW_SCH_EN, xdpq->complq);
>> +		idpf_queue_set(NOIRQ, xdpq);
>> +		idpf_queue_set(XDP, xdpq);
>> +		idpf_queue_set(XDP, xdpq->complq);
>> +
>> +		xdpq->timer = timers[i - sqs];
>> +		libeth_xdpsq_get(&xdpq->xdp_lock, dev, vport->xdpq_share);
>> +
>> +		xdpq->pending = 0;
>> +		xdpq->xdp_tx = 0;
>> +		xdpq->thresh = libeth_xdp_queue_threshold(xdpq->desc_count);
>> +	}
>> +
>> +	return 0;
>> +}

Thanks,
Olek