netdev - Re: [PATCH bpf-next v4 07/17] libbpf: Support drivers with non-combined channels

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 Jun 2019 13:25:24 +0000
From:   Maxim Mikityanskiy <maximmi@...lanox.com>
To:     Maciej Fijalkowski <maciejromanfijalkowski@...il.com>
CC:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        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>
Subject: Re: [PATCH bpf-next v4 07/17] libbpf: Support drivers with
 non-combined channels

On 2019-06-13 17:45, Maciej Fijalkowski wrote:
> On Thu, 13 Jun 2019 14:01:39 +0000
> Maxim Mikityanskiy <maximmi@...lanox.com> 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()).
>>
>> The formula in libbpf should match it.
>>
>> 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.
>>
>>>>    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);
> 
> So in case of 32 HW queues you'd like to get 64 entries in xskmap?

"32 HW queues" is not quite correct. It will be 32 combined channels, 
each with one regular RX queue and one XSK RX queue (regular RX queues 
are part of RSS). In this case, I'll have 64 XSKMAP entries.

> Do you still
> have a need for attaching the xsksocks to the RSS queues?

You can attach an XSK to a regular RX queue, but not in zero-copy mode. 
The intended use is, of course, to attach XSKs to XSK RX queues in 
zero-copy mode.

> I thought you want
> them to be separated. So if I'm reading this right, [0, 31] xskmap entries
> would be unused for the most of the time, no?

This is correct, but these entries are still needed if one decides to 
run compatibility mode without zero-copy on queues 0..31.

> 
>>>> +
>>>> +	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);
>>>    
>>
>