[PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yunsheng Lin]

This patchset adds a new hardware offload type in mqprio before adding
mqprio hardware offload support in hns3 driver.

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

[PATCH net-next 1/2] mqprio: Add a new hardware offload type in mqprio

[Yunsheng Lin]

When a driver supports both dcb and hardware offloaded mqprio, and
user is running mqprio and dcb tool concurrently, the configuration
set by each tool may be conflicted with each other because the dcb
and mqprio may be using the same hardwere offload component and share
the tc system in the network stack.

This patch adds a new offload type to indicate that the underlying
driver offload 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 qdisc offload.

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
Suggested-by: Yuval Mintz <yuvalm@mellanox.com>
---
 include/uapi/linux/pkt_sched.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 099bf55..8016027 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -620,6 +620,7 @@ struct tc_drr_stats {
 enum {
 	TC_MQPRIO_HW_OFFLOAD_NONE,	/* no offload requested */
 	TC_MQPRIO_HW_OFFLOAD_TCS,	/* offload TCs, no queue counts */
+	TC_MQPRIO_HW_OFFLOAD_DCB,	/* offload shared by DCB */
 	__TC_MQPRIO_HW_OFFLOAD_MAX
 };
 
-- 
1.9.1

RE: [PATCH net-next 1/2] mqprio: Add a new hardware offload type in mqprio

[Yuval Mintz]

> When a driver supports both dcb and hardware offloaded mqprio, and
> user is running mqprio and dcb tool concurrently, the configuration
> set by each tool may be conflicted with each other because the dcb
(for second 'each') s/each/the

> and mqprio may be using the same hardwere offload component and share
s/hardwere/hardware

> the tc system in the network stack.
> 
> This patch adds a new offload type to indicate that the underlying
> driver offload prio mapping as part of DCB. If the driver would be
'should' offload

> incapable of that it would refuse the offload. User would then have
> to explicitly request that qdisc offload.

Re: [PATCH net-next 1/2] mqprio: Add a new hardware offload type in mqprio

[Yunsheng Lin]

Hi, Yuval

On 2017/10/13 4:10, Yuval Mintz wrote:
>> When a driver supports both dcb and hardware offloaded mqprio, and
>> user is running mqprio and dcb tool concurrently, the configuration
>> set by each tool may be conflicted with each other because the dcb
> (for second 'each') s/each/the
> 

Will do, Thanks

>> and mqprio may be using the same hardwere offload component and share
> s/hardwere/hardware

Will do, Thanks

> 
>> the tc system in the network stack.
>>
>> This patch adds a new offload type to indicate that the underlying
>> driver offload prio mapping as part of DCB. If the driver would be
> 'should' offload

Will do, Thanks

> 
>> incapable of that it would refuse the offload. User would then have
>> to explicitly request that qdisc offload.
> 
> 
> 

[PATCH net-next 2/2] net: hns3: Add mqprio hardware offload support in hns3 driver

[Yunsheng Lin]

When using tc qdisc, dcb_ops->setup_tc is used to tell hclge_dcb
module to do the tm related setup. Only TC_MQPRIO_HW_OFFLOAD_DCB
offload type is supported.

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 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 ++++++++++++++--------
 3 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index 575f50d..3acd8db 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -381,6 +381,7 @@ struct hnae3_dcb_ops {
 	u8   (*setdcbx)(struct hnae3_handle *, u8);
 
 	int (*map_update)(struct hnae3_handle *);
+	int (*setup_tc)(struct hnae3_handle *, u8, u8 *);
 };
 
 struct hnae3_ae_algo {
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
index 1b30a6f..7ec9484 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
@@ -276,6 +276,28 @@ static u8 hclge_setdcbx(struct hnae3_handle *h, u8 mode)
 	return 0;
 }
 
+static int hclge_setup_tc(struct hnae3_handle *h, u8 tc, u8 *prio_tc)
+{
+	struct hclge_vport *vport = hclge_get_vport(h);
+	struct hclge_dev *hdev = vport->back;
+	int ret;
+
+	if (tc > hdev->tc_max) {
+		dev_err(&hdev->pdev->dev,
+			"setup tc failed, tc(%u) > tc_max(%u)\n",
+			tc, hdev->tc_max);
+		return -EINVAL;
+	}
+
+	hclge_tm_schd_info_update(hdev, tc);
+
+	ret = hclge_tm_prio_tc_info_update(hdev, prio_tc);
+	if (ret)
+		return ret;
+
+	return hclge_tm_init_hw(hdev);
+}
+
 static const struct hnae3_dcb_ops hns3_dcb_ops = {
 	.ieee_getets	= hclge_ieee_getets,
 	.ieee_setets	= hclge_ieee_setets,
@@ -284,6 +306,7 @@ static u8 hclge_setdcbx(struct hnae3_handle *h, u8 mode)
 	.getdcbx	= hclge_getdcbx,
 	.setdcbx	= hclge_setdcbx,
 	.map_update	= hclge_map_update,
+	.setup_tc	= hclge_setup_tc,
 };
 
 void hclge_dcb_ops_set(struct hclge_dev *hdev)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
index ba550c1..79d8d6b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
@@ -1186,42 +1186,56 @@ static void hns3_nic_udp_tunnel_del(struct net_device *netdev,
 	}
 }
 
-static int hns3_setup_tc(struct net_device *netdev, u8 tc)
+static int hns3_setup_tc(struct net_device *netdev, u8 tc, u8 *prio_tc)
 {
 	struct hnae3_handle *h = hns3_get_handle(netdev);
 	struct hnae3_knic_private_info *kinfo = &h->kinfo;
+	bool if_running;
 	unsigned int i;
 	int ret;
 
 	if (tc > HNAE3_MAX_TC)
 		return -EINVAL;
 
-	if (kinfo->num_tc == tc)
-		return 0;
-
 	if (!netdev)
 		return -EINVAL;
 
-	if (!tc) {
-		netdev_reset_tc(netdev);
-		return 0;
+	if_running = netif_running(netdev);
+	if (if_running) {
+		hns3_nic_net_stop(netdev);
+		msleep(100);
 	}
 
-	/* Set num_tc for netdev */
-	ret = netdev_set_num_tc(netdev, tc);
+	ret = (kinfo->dcb_ops && kinfo->dcb_ops->setup_tc) ?
+		kinfo->dcb_ops->setup_tc(h, tc, prio_tc) : -EOPNOTSUPP;
 	if (ret)
-		return ret;
+		goto out;
+
+	if (tc <= 1) {
+		netdev_reset_tc(netdev);
+	} else {
+		ret = netdev_set_num_tc(netdev, tc);
+		if (ret)
+			goto out;
+
+		for (i = 0; i < HNAE3_MAX_TC; i++) {
+			if (!kinfo->tc_info[i].enable)
+				continue;
 
-	/* Set per TC queues for the VSI */
-	for (i = 0; i < HNAE3_MAX_TC; i++) {
-		if (kinfo->tc_info[i].enable)
 			netdev_set_tc_queue(netdev,
 					    kinfo->tc_info[i].tc,
 					    kinfo->tc_info[i].tqp_count,
 					    kinfo->tc_info[i].tqp_offset);
+		}
 	}
 
-	return 0;
+	ret = hns3_nic_set_real_num_queue(netdev);
+
+out:
+	if (if_running)
+		hns3_nic_net_open(netdev);
+
+	return ret;
 }
 
 static int hns3_nic_setup_tc(struct net_device *dev, enum tc_setup_type type,
@@ -1229,10 +1243,10 @@ static int hns3_nic_setup_tc(struct net_device *dev, enum tc_setup_type type,
 {
 	struct tc_mqprio_qopt *mqprio = type_data;
 
-	if (type != TC_SETUP_MQPRIO)
+	if (type != TC_SETUP_MQPRIO || mqprio->hw != TC_MQPRIO_HW_OFFLOAD_DCB)
 		return -EOPNOTSUPP;
 
-	return hns3_setup_tc(dev, mqprio->num_tc);
+	return hns3_setup_tc(dev, mqprio->num_tc, mqprio->prio_tc_map);
 }
 
 static int hns3_vlan_rx_add_vid(struct net_device *netdev,
-- 
1.9.1

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

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

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]

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

Re: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yunsheng Lin]

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

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

Maybe we can add a callback to notify mqprio the configuration has changed.

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
> 
> 
> 

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

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

> 
> >
> > 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]

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

> 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
> >
> >
> >

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

> > >> This patchset adds a new hardware offload type in mqprio before
> adding
> > >> mqprio hardware offload support in hns3 driver.

Apparently Dave has already accepted 	Amirtha's changes to mqprio:
https://marc.info/?l=linux-netdev&m=150803219824053&w=2 
so I guess you need to revise your patchs to align to the new conventions.

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

Re: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yunsheng Lin]

Hi, Yuval

On 2017/10/15 16:51, Yuval Mintz wrote:
>>>>> This patchset adds a new hardware offload type in mqprio before
>> adding
>>>>> mqprio hardware offload support in hns3 driver.
> 
> Apparently Dave has already accepted 	Amirtha's changes to mqprio:
> https://marc.info/?l=linux-netdev&m=150803219824053&w=2 
> so I guess you need to revise your patchs to align to the new conventions.

Ok.

"If offloads are supported by setting the 'hw' option to 1, the default
offload mode is 'dcb' where only the TC values are offloaded to the
device. "

According to the description of the above patchset, the default mode is already
dcb, so i will drop the dcb mode patch.

I think the scenario you mentioned still existed, and I am willing to implement
it if we come to a solution that will suit most in the community.

Thanks,
Yunsheng Lin

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

Re: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yunsheng Lin]

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
>>>
>>>
>>>
> 

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

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

So there is a qdisc attached - mqprio_attach() attches to all transmission
queues [num_tx_queues] and not only the active ones.
But the flow for mqprio might be lacking the additional qdisc_hash_add()
for the additional queue's qdisc.

> 
> >
> >>
> >> 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
> >>>
> >>>
> >>>
> >

Re: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yunsheng Lin]

Hi, Yuval

On 2017/10/16 14:25, Yuval Mintz wrote:
>> 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.
> 
> So there is a qdisc attached - mqprio_attach() attches to all transmission
> queues [num_tx_queues] and not only the active ones.
> But the flow for mqprio might be lacking the additional qdisc_hash_add()
> for the additional queue's qdisc.

Yes, I think you may be right.

static void mqprio_attach(struct Qdisc *sch)
{
	struct net_device *dev = qdisc_dev(sch);
	struct mqprio_sched *priv = qdisc_priv(sch);
	struct Qdisc *qdisc, *old;
	unsigned int ntx;

	/* Attach underlying qdisc */
	for (ntx = 0; ntx < dev->num_tx_queues; ntx++) {
		qdisc = priv->qdiscs[ntx];
		old = dev_graft_qdisc(qdisc->dev_queue, qdisc);
		if (old)
			qdisc_destroy(old);

----------Only call qdisc_hash_add when  ntx < dev->real_num_tx_queues---------------

		if (ntx < dev->real_num_tx_queues)
			qdisc_hash_add(qdisc, false);
	}
	kfree(priv->qdiscs);
	priv->qdiscs = NULL;
}


> 
>>
>>>
>>>>
>>>> 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
>>>>>
>>>>>
>>>>>
>>>
>