From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751693AbdJPBm2 (ORCPT ); Sun, 15 Oct 2017 21:42:28 -0400 Received: from szxga05-in.huawei.com ([45.249.212.191]:8047 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751498AbdJPBm0 (ORCPT ); Sun, 15 Oct 2017 21:42:26 -0400 Subject: Re: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver To: Yuval Mintz , "davem@davemloft.net" CC: "huangdaode@hisilicon.com" , "xuwei5@hisilicon.com" , "liguozhu@hisilicon.com" , "Yisen.Zhuang@huawei.com" , "gabriele.paoloni@huawei.com" , "john.garry@huawei.com" , "linuxarm@huawei.com" , "salil.mehta@huawei.com" , "lipeng321@huawei.com" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <1507775912-22402-1-git-send-email-linyunsheng@huawei.com> <5c268e80-3b04-28bd-26fb-d77f2edd69a8@huawei.com> From: Yunsheng Lin Message-ID: Date: Mon, 16 Oct 2017 09:41:58 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.74.191.121] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020203.59E40E75.0118,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 6368577a0de4ce8de4a24c6eb0d7e2a0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Yuval On 2017/10/15 13:14, Yuval Mintz wrote: >> Hi, Yuval >> >> On 2017/10/13 4:21, Yuval Mintz wrote: >>>> This patchset adds a new hardware offload type in mqprio before adding >>>> mqprio hardware offload support in hns3 driver. >>> >>> I think one of the biggest issues in tying this to DCB configuration is the >>> non-immediate [and possibly non persistent] configuration. >>> >>> Scenario #1: >>> User is configuring mqprio offloaded with 3 TCs while device is in willing >> mode. >>> Would you expect the driver to immediately respond with a success or >> instead >>> delay the return until the DCBx negotiation is complete and the operational >>> num of TCs is actually 3? >> >> Well, when user requsts the mqprio offloaded by a hardware shared by DCB, >> I expect >> the user is not using the dcb tool. >> If user is still using dcb tool, then result is undefined. >> >> The scenario you mention maybe can be enforced by setting willing to zero >> when user >> is requesting the mqprio offload, and restore the willing bit when unloaded >> the mqprio >> offload. > > Sounds a bit harsh but would probably work. > >> But I think the real issue is that dcb and mqprio shares the tc system in the >> stack, >> the problem may be better to be fixed in the stack rather than in the driver, >> as you >> suggested in the DCB patchset. What do you think? > > What did you have in mind? I was thinking maybe the tc system can provide a notification to mqprio and dcb. mqprio and dcb register a callback to the tc system, when there is some change of tc configuration, the tc system call the callback from mqprio and dcb. > >> >>> >>> Scenario #2: >>> Assume user explicitly offloaded mqprio with 3 TCs, but now DCB >> configuration >>> has changed on the peer side and 4 TCs is the new negotiated operational >> value. >>> Your current driver logic would change the number of TCs underneath the >> user >>> configuration [and it would actually probably work due to mqprio being a >> crappy >>> qdisc]. But was that the user actual intention? >>> [I think the likely answer in this scenario is 'yes' since the alternative is no >> better. >>> But I still thought it was worth mentioning] >> >> You are right, the problem also have something to do with mqprio and dcb >> sharing >> the tc in the stack. >> >> Druing testing, when user explicitly offloaded mqprio with 3 TCs, all >> queue has a default pfifo mqprio attached, after DCB changes the tc num to >> 4, >> using tc qdisc shows some queue does not have a default pfifo mqprio >> attached. > > Really? Then what did it show? > [I assume it has some pfifo attached, and it's an mqprio dump kind of an issue] When queue size of the ndev is 16 and tc num is 3, we set the real queue size to 15 ( 5 * 3 = 15), mqprio only attach pfifo to the first 15 queue, when tc num change to 4 by DCB, we set the real queue size to 16 (4 * 4 = 16). So tc qdisc shows the last queue has no qdisc attached. > >> >> Maybe we can add a callback to notify mqprio the configuration has changed. >> > > Which would do what? > You already have the notifications available for monitoring using dcbnl logic if the > configuration change [for user]; So user can re-configure whatever it wants. Yes, if user is only using dcb tool. > But other than dropping all the qdisc configurations and going back to the default > qdiscs, what default action would mqprio be able to do when configuration changes > that actually makes sense? As explained above, after dcb changing the configuration, some queue may have no qdisc attached, so I was thinking maybe we can add pfifo to it if there is no qdsic attached to it. Thanks, Yunsheng Lin > >> Thanks >> Yunsheng Lin >> >>> >>> Cheers, >>> Yuval >>> >>>> >>>> Yunsheng Lin (2): >>>> mqprio: Add a new hardware offload type in mqprio >>>> net: hns3: Add mqprio hardware offload support in hns3 driver >>>> >>>> drivers/net/ethernet/hisilicon/hns3/hnae3.h | 1 + >>>> .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23 +++++++++++ >>>> .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46 >> ++++++++++++++- >>>> ------- >>>> include/uapi/linux/pkt_sched.h | 1 + >>>> 4 files changed, 55 insertions(+), 16 deletions(-) >>>> >>>> -- >>>> 1.9.1 >>> >>> >>> >