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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Mon, Mar 17, 2025 at 03:50:11PM +0100, Alexander Lobakin wrote:
> 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

I meant to say something in commit message how new output should be
interpreted?

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

ack, thanks

> 
> [...]
> 
> >> +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).

so this array is needed to ease the error path handling?

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