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

[Yunsheng Lin]

Hi, Yuval

On 2017/9/26 20:29, Yuval Mintz wrote:
>> 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.
>>
>> In net/dcb/dcbnl.c
>> dcbnl_ieee_set already call dcbnl_ieee_notify to notify the user space
>> configuration has changed, does this dcbnl_ieee_notify function do the
>> job for us? I am not sure if lldpad has registered for this notifition.
> 
> Not that familiar with the dcbnl calls; Shouldn't dcbnl_setall be called to
> make the configuration apply [or is that only for ieee]?

dcbnl_setall is for cee to make the configuration apply.
ieee does not have the apply operation.

> Regardless, don't know if it makes sense to assume user-application would
> fix the qdisc configuration by notification while dcbnl logic in kernel could have
> done that instead.
> 
>> As you suggested below, can we add a new TC_MQPRIO_HW_OFFLOAD_
>> value to
>> reflect that the configuration is needed to be changed by dcbnl_ieee_set
>> (perhaps some other function) in dcbnl?
>> Do you think it is feasible?
> 
> Either I'm miseading your answer or we think of it from 2 opposite end.
> I was thinking that the new offloaded flag would indicate to the underlying
> driver that it's expected to offload the prio mapping [as part of DCB].
> If the driver would be incapable of that it would refuse the offload.
> User would then have to explicitly request that the qdisc offload.


Adding a new offloaded flag to indicate that mqpri is using a hardware offload
shared by dcbnl seems a good idea.
As I do not know how the idea go with other, I will drop the mqprio support in
this patch, and try to add the mqprio support as you suggested in the next
patchset.

Thanks again for the lengthly reply.

> 
>>
>>
>>>   - 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?
>>>
>>> ...
>>>
>>>> +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?
>>>
>>> .
>>>
>