netdev - Re: [PATCH v2 net-next 10/10] net: hns3: Add mqprio support when interacting with network stack

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 26 Sep 2017 15:29:03 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     Yuval Mintz <yuvalm@...lanox.com>
CC:     "huangdaode@...ilicon.com" <huangdaode@...ilicon.com>,
        "xuwei5@...ilicon.com" <xuwei5@...ilicon.com>,
        "liguozhu@...ilicon.com" <liguozhu@...ilicon.com>,
        "Yisen.Zhuang@...wei.com" <Yisen.Zhuang@...wei.com>,
        "gabriele.paoloni@...wei.com" <gabriele.paoloni@...wei.com>,
        "john.garry@...wei.com" <john.garry@...wei.com>,
        "linuxarm@...wei.com" <linuxarm@...wei.com>,
        "salil.mehta@...wei.com" <salil.mehta@...wei.com>,
        "lipeng321@...wei.com" <lipeng321@...wei.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v2 net-next 10/10] net: hns3: Add mqprio support when
 interacting with network stack

Hi, Yuval

On 2017/9/26 14:43, Yuval Mintz wrote:
>> When using tc qdisc to configure DCB parameter, dcb_ops->setup_tc
>> is used to tell hclge_dcb module to do the setup.
> 
> While this might be a step in the right direction, this causes an inconsistency
> in user experience - Some [well, most] vendors didn't allow the mqprio
> priority mapping to affect DCB, instead relying on the dcbnl functionality
> to control that configuration.
> 
> A couple of options to consider:
>   - Perhaps said logic shouldn't be contained inside the driver but rather
>      in mqprio logic itself. I.e., rely on DCBNL functionality [if available] from
>      within mqprio and try changing the configuration. 
>   - Add a new TC_MQPRIO_HW_OFFLOAD_ value to explicitly reflect user
>      request to allow this configuration to affect DCB.
> 
>> When using lldptool to configure DCB parameter, hclge_dcb module
>> call the client_ops->setup_tc to tell network stack which queue
>> and priority is using for specific tc.
> 
> You're basically bypassing the mqprio logic.
> Since you're configuring the prio->queue mapping from DCB flow,
> you'll get an mqprio-like behavior [meaning a transmitted packet
> would reach a transmission queue associated with its priority] even
> if device wasn't grated with an mqprio qdisc.
> Why should your user even use mqprio? What benefit does he get from it?


When adding mqprio and lldptool support, I was thinking user can use
tc qdisc or lldptool to do the configuration, giving user two option to
setup the DCB.

If user is only tc qdisc or lldptool, I think there is no problem here.

when user is using tc qdisc and lldptool, As you explained above, When
tc qdisc changes the configuration, there should be a way to notify dcbnl,
so that the dcbnl can response correctly(like tell the peer it's configuration
has changed).

I will try to find if there is a way to do notify the dcbnl when using tc qdisc
to setup the configuration.
If there is not a way to do it now, then I will drop the mqprio in this patch, and
will address this problem if there is need for the tc qdisc.

Please let me know if I was misunderstood.
And thanks for your time reviewing.

> 
> ...
> 
>> +static int hns3_nic_set_real_num_queue(struct net_device *netdev)
>> +{
>> +	struct hns3_nic_priv *priv = netdev_priv(netdev);
>> +	struct hnae3_handle *h = priv->ae_handle;
>> +	struct hnae3_knic_private_info *kinfo = &h->kinfo;
>> +	unsigned int queue_size = kinfo->rss_size * kinfo->num_tc;
>> +	int ret;
>> +
>> +	ret = netif_set_real_num_tx_queues(netdev, queue_size);
>> +	if (ret) {
>> +		netdev_err(netdev,
>> +			   "netif_set_real_num_tx_queues fail, ret=%d!\n",
>> +			   ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = netif_set_real_num_rx_queues(netdev, queue_size);
> 
> I don't think you're changing the driver behavior, but why are you setting
> the real number of rx queues based on the number of TCs?
> Do you actually open (TC x RSS) Rx queues?
> 
> .
>