Date:   Fri, 14 Jun 2019 13:25:05 +0000
From:   Maxim Mikityanskiy <maximmi@...lanox.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Björn Töpel <bjorn.topel@...el.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Jonathan Lemon <bsd@...com>,
        Tariq Toukan <tariqt@...lanox.com>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Maciej Fijalkowski <maciejromanfijalkowski@...il.com>
Subject: Re: [PATCH bpf-next v4 07/17] libbpf: Support drivers with
 non-combined channels

On 2019-06-13 21:09, Jakub Kicinski wrote:
> On Thu, 13 Jun 2019 14:01:39 +0000, Maxim Mikityanskiy wrote:
>> On 2019-06-12 23:23, Jakub Kicinski wrote:
>>> On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote:
>>>> Currently, libbpf uses the number of combined channels as the maximum
>>>> queue number. However, the kernel has a different limitation:
>>>>
>>>> - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues).
>>>>
>>>> - ethtool_set_channels() checks for UMEMs in queues up to
>>>>     combined_count + max(rx_count, tx_count).
>>>>
>>>> libbpf shouldn't limit applications to a lower max queue number. Account
>>>> for non-combined RX and TX channels when calculating the max queue
>>>> number. Use the same formula that is used in ethtool.
>>>>
>>>> Signed-off-by: Maxim Mikityanskiy <maximmi@...lanox.com>
>>>> Reviewed-by: Tariq Toukan <tariqt@...lanox.com>
>>>> Acked-by: Saeed Mahameed <saeedm@...lanox.com>
>>>
>>> I don't think this is correct.  max_tx tells you how many TX channels
>>> there can be, you can't add that to combined.  Correct calculations is:
>>>
>>> max_num_chans = max(max_combined, max(max_rx, max_tx))
>>
>> First of all, I'm aligning with the formula in the kernel, which is:
>>
>>       curr.combined_count + max(curr.rx_count, curr.tx_count);
>>
>> (see net/core/ethtool.c, ethtool_set_channels()).
> 
> curr != max.  ethtool code you're pointing me to (and which I wrote)
> uses the current allocation, not the max values.

The ethtool code uses curr, because it wants to calculate the amount of 
queues currently in use. libbpf uses max, because it wants to calculate 
the maximum amount of queues that can be in use. That's the only 
difference, so the formula should be the same, and this calculation can 
be applied either to curr or to max.

Imagine you have configured the NIC to have the maximum supported amount 
of channels. Then your formula in ethtool.c returns some value. Exactly 
the same value should also be returned from libbpf's 
xsk_get_max_queues(). It's achieved by applying your formula directly to 
max.

>> The formula in libbpf should match it.
> 
> The formula should be based on understanding what we're doing,
> not copying some not-really-equivalent code from somewhere :)

I have understanding of the code I write.

> Combined is a basically a queue pair, RX is an RX ring with a dedicated
> IRQ, and TX is a TX ring with a dedicated IRQ.  If driver supports both
> combined and single purpose interrupt vectors it will most likely set
> 
> 	max_rx = num_hw_rx
> 	max_tx = num_hw_tx
> 	max_combined = min(rx, tx)

OK, I got your example. The driver you are talking about won't support 
setting rx_count = max_rx, tx_count = max_tx and combined_count = 
max_combined simultaneously.

However, xsk_get_max_queues has to return the maximum number of queues 
theoretically possible with this device, to create xsks_map of a 
sufficient size. Currently, it totally ignores devices without combined 
channels, so max_rx and max_tx have to be considered in the calculation. 
Next thing is that ethtool API doesn't really tell you whether the 
device can create up to max_rx RX channels, max_tx TX channels and 
max_combined combined channels simultaneously, or there are some 
additional limitations. Your example displays such a limitation, but 
it's not the only possible one, and this limitation is not even 
mandatory for all drivers. As ethtool doesn't expose the information 
about additional limitations imposed by the driver, and as it won't hurt 
if xsks_map will be bigger than necessary, my vision is that we 
shouldn't assume any limitations we are not sure about, so max_combined 
+ max(max_rx, max_tx) is the right thing to do.

> Like with most ethtool APIs there are some variations to this.
> 
>> Second, the existing drivers have either combined channels or separate
>> rx and tx channels. So, for the first kind of drivers, max_tx doesn't
>> tell how many TX channels there can be, it just says 0, and max_combined
>> tells how many TX and RX channels are supported. As max_tx doesn't
>> include max_combined (and vice versa), we should add them up.
> 
> By existing drivers you mean Intel drivers which implement AF_XDP,
> and your driver?

No, I meant all drivers, not only AF_XDP-enabled ones. I wasn't aware 
that some of them support the choice between a combined channel and a 
unidirectional channel, however, I still find my formula correct (see 
the explanation above).

> Both Intel and MLX drivers seem to only set
> max_combined.
mlx4 doesn't support combined channels, but it's out of scope of this 
patchset.

> If you mean all drivers across the kernel, then I believe the best
> formula is what I gave you.
> 
>>>>    tools/lib/bpf/xsk.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
>>>> index bf15a80a37c2..86107857e1f0 100644
>>>> --- a/tools/lib/bpf/xsk.c
>>>> +++ b/tools/lib/bpf/xsk.c
>>>> @@ -334,13 +334,13 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
>>>>    		goto out;
>>>>    	}
>>>>    
>>>> -	if (channels.max_combined == 0 || errno == EOPNOTSUPP)
>>>> +	ret = channels.max_combined + max(channels.max_rx, channels.max_tx);
>>>> +
>>>> +	if (ret == 0 || errno == EOPNOTSUPP)
>>>>    		/* If the device says it has no channels, then all traffic
>>>>    		 * is sent to a single stream, so max queues = 1.
>>>>    		 */
>>>>    		ret = 1;
>>>> -	else
>>>> -		ret = channels.max_combined;
>>>>    
>>>>    out:
>>>>    	close(fd);

Powered by blists - more mailing lists