[email protected] writes:
>> 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?
Yup. I'll send out a separate fix for this.
>> @@ -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?
Hmm, yes, it seems like this is a valid bug, when fbnic's real queue
counts and private counts disagree! I fixed the root cause by keeping
the real queue count synchronized. I'll send this one out separately.
Björn
