On Mon, Mar 17, 2025 at 03:50:11PM +0100, Alexander Lobakin wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> 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@xxxxxxxxx>
> >>
> >> 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
