Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0
- [High] fbnic_alloc_rx_qt_resources() programs the PPQ device-page
geometry unconditionally (`fbnic_bdq_set_page_size(&qt->sub1,…
Pre-existing issues:
- [High] In fbnic_alloc_qt_page_pools()
(drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:1611-1623) the local…
--
fbnic: Support larger memory-provider RX pages
The patch lets memory providers pick a larger RX page size for a queue via
QCFG_RX_PAGE_SIZE, uses that rendered rx_page_size for the PPQ page-pool
allocations and completion decoding, and adds ndo_default_qcfg /
ndo_validate_qcfg callbacks to sanity check the requested size.
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 28b962307c080..5e22905a63739 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
[ ... ]
> @@ -1599,6 +1603,8 @@ fbnic_alloc_qt_page_pools(struct fbnic_net *fbn, struct
> fbnic_q_triad *qt,
>
> qt->sub0.page_pool = pp;
> if (netif_rxq_has_unreadable_mp(fbn->netdev, rxq_idx)) {
> + pp_params.order = get_order(rx_page_size);
> + pp_params.max_len = rx_page_size;
> pp_params.flags |= PP_FLAG_ALLOW_UNREADABLE_NETMEM;
> pp_params.dma_dir = DMA_FROM_DEVICE;
>
> pp = page_pool_create(&pp_params);
> if (IS_ERR(pp))
> goto err_destroy_sub0;
[Severity: High]
This isn't a bug introduced by this patch, but the patch widens the set of
inputs that can make this page_pool_create() fail, so it seems worth
mentioning.
At the err_destroy_sub0 label, pp has already been reassigned to the
ERR_PTR returned by the second page_pool_create():
err_destroy_sub0:
page_pool_destroy(pp);
return PTR_ERR(pp);
page_pool_destroy() only short-circuits on NULL:
net/core/page_pool.c:page_pool_destroy() {
if (!pool)
return;
if (!page_pool_put(pool))
return;
...
}
and page_pool_put() then does refcount_dec_and_test(&pool->user_cnt) on
the error pointer. Should the label destroy qt->sub0.page_pool instead,
and save the error value before overwriting pp?
Along the same path, is qt->sub0.page_pool leaked together with its DMA
mappings? fbnic_alloc_rx_qt_resources() returns the error before reaching
its free_page_pools label, fbnic_alloc_nv_resources()'s "while (i--)"
unwind skips the failing qt, and on the queue-reconfig side
netdev_rx_queue_reconfig() goes to err_free_old_mem (kvfree of the qmem
container only, no ndo_queue_mem_free) when ndo_queue_mem_alloc fails.
The second create can fail for ordinary reasons: page_pool_init() returns
-ENOMEM for the ptr_ring allocation (up to 16384 entries here) and can
also propagate an error from pool->mp_ops->init(pool).
[ ... ]
> @@ -2028,15 +2034,18 @@ static int fbnic_alloc_tx_qt_resources(struct
> fbnic_net *fbn,
>
> static int fbnic_alloc_rx_qt_resources(struct fbnic_net *fbn,
> struct fbnic_napi_vector *nv,
> - struct fbnic_q_triad *qt)
> + struct fbnic_q_triad *qt,
> + u32 rx_page_size)
> {
> struct device *dev = fbn->netdev->dev.parent;
> int err;
>
> - err = fbnic_alloc_qt_page_pools(fbn, qt, qt->cmpl.q_idx);
> + err = fbnic_alloc_qt_page_pools(fbn, qt, qt->cmpl.q_idx, rx_page_size);
> if (err)
> return err;
>
> + fbnic_bdq_set_page_size(&qt->sub1, rx_page_size);
> +
[Severity: High]
Can the BDQ geometry here end up describing a page pool that was never
enlarged?
fbnic_bdq_set_page_size(&qt->sub1, rx_page_size) runs unconditionally,
but fbnic_alloc_qt_page_pools() only raises order/max_len when
netif_rxq_has_unreadable_mp() is true; otherwise sub1 just takes another
reference on sub0's order-0, PAGE_SIZE pool. The two predicates are not
the same:
net/core/netdev_rx_queue.c:netif_rxq_has_unreadable_mp() {
if (rxq_idx < dev->real_num_rx_queues)
return __netif_get_rx_queue(dev,
rxq_idx)->mp_params.mp_ops;
return false;
}
net/core/netdev_config.c:__netdev_queue_config() {
mpp = &__netif_get_rx_queue(dev, rxq_idx)->mp_params;
if (mpp->rx_page_size)
qcfg->rx_page_size = mpp->rx_page_size;
}
The config renderer applies the memory provider's rx_page_size with no
real_num_rx_queues bound check, while netif_rxq_has_unreadable_mp()
silently reports false above real_num_rx_queues.
When they disagree, does the device get descriptors past the end of the
mapping? fbnic_bd_prep() posts fbnic_bd_page_count() descriptors from a
single allocation:
__le64 *bdq_desc = &bdq->desc[id * fbnic_bd_page_count(bdq)];
dma_addr_t dma = page_pool_get_dma_addr_netmem(netmem);
u64 bd, i = fbnic_bd_page_count(bdq);
and on the completion side fbnic_rcd_bd_page_offset() decodes offsets up
to rx_page_size - 1, which fbnic_add_rx_frag() then feeds to
page_pool_dma_sync_netmem_for_cpu() and xdp_buff_add_frag() as payload
inside what is really a PAGE_SIZE buffer.
A path that seems to reach this: fbnic only sets real_num_rx_queues in
fbnic_set_netif_queues() at open, so after probe it equals
FBNIC_MAX_RXQS from alloc_etherdev_mq(). A devmem/io_uring bind with
rx-page-size=8K against a high queue index is accepted then, "ip link set
up" shrinks real_num_rx_queues (netif_set_real_num_rx_queues() has no
memory-provider check), and a later "ethtool -L combined 32" makes
fbnic_alloc_nv_resources() allocate that queue while
netif_rxq_has_unreadable_mp() returns false but qcfg.rx_page_size is
still 8192.
Before this patch bd_page_shift was always derived from PAGE_SIZE and so
always matched the order-0 pool. Would it make sense to either derive the
BDQ page size from the pool that was actually created, or reject
rx_page_size != PAGE_SIZE when no large-order pool was allocated?
> err = xdp_rxq_info_reg(&qt->xdp_rxq, fbn->netdev, qt->sub0.q_idx,
> nv->napi.napi_id);
> if (err)
[ ... ]
> @@ -2097,7 +2106,11 @@ static int fbnic_alloc_nv_resources(struct fbnic_net
> *fbn,
>
> /* Allocate Rx Resources */
> for (j = 0; j < nv->rxt_count; j++, i++) {
> - err = fbnic_alloc_rx_qt_resources(fbn, nv, &nv->qt[i]);
> + struct netdev_queue_config qcfg;
> +
> + netdev_queue_config(fbn->netdev, nv->qt[i].cmpl.q_idx, &qcfg);
> + err = fbnic_alloc_rx_qt_resources(fbn, nv, &nv->qt[i],
> + qcfg.rx_page_size);
> if (err)
> goto free_qt_resources;
> }
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910180908.1506533-1-bjorn%40kernel.org
