Re: [PATCH net-next v2 4/5] fbnic: Support larger memory-provider RX pages

[Björn Töpel]

netdev-bot+sashiko@xxxxxxxxxx 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