| Message ID | 1506392718-50463-11-git-send-email-linyunsheng@huawei.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add support for DCB feature in hns3 driver | expand |
> 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? ... > +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?
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? > > . >
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? > > ... > >> +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? Yes, our hardware can do the rss based on TC. Sorry for almost forget to answer this question. Thanks for your time reviewing again. > > . >
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. 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? > - 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? > > . >
> 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]? 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. > > > > - 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? > > > > . > >
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? >>> >>> . >>> >
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c index 11dab26..31fcda4 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c @@ -196,6 +196,32 @@ static void hns3_vector_gl_rl_init(struct hns3_enet_tqp_vector *tqp_vector) tqp_vector->tx_group.flow_level = HNS3_FLOW_LOW; } +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); + if (ret) { + netdev_err(netdev, + "netif_set_real_num_rx_queues fail, ret=%d!\n", ret); + return ret; + } + + return 0; +} + static int hns3_nic_net_up(struct net_device *netdev) { struct hns3_nic_priv *priv = netdev_priv(netdev); @@ -232,26 +258,13 @@ static int hns3_nic_net_up(struct net_device *netdev) static int hns3_nic_net_open(struct net_device *netdev) { - struct hns3_nic_priv *priv = netdev_priv(netdev); - struct hnae3_handle *h = priv->ae_handle; int ret; netif_carrier_off(netdev); - ret = netif_set_real_num_tx_queues(netdev, h->kinfo.num_tqps); - 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, h->kinfo.num_tqps); - if (ret) { - netdev_err(netdev, - "netif_set_real_num_rx_queues fail, ret=%d!\n", ret); + ret = hns3_nic_set_real_num_queue(netdev); + if (ret) return ret; - } ret = hns3_nic_net_up(netdev); if (ret) { @@ -1193,32 +1206,40 @@ 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 hns3_nic_priv *priv = netdev_priv(netdev); struct hnae3_handle *h = priv->ae_handle; struct hnae3_knic_private_info *kinfo = &h->kinfo; + bool if_running = netif_running(netdev); 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) { + if (if_running) { + (void)hns3_nic_net_stop(netdev); + msleep(100); + } + + ret = (kinfo->dcb_ops && kinfo->dcb_ops->setup_tc) ? + kinfo->dcb_ops->setup_tc(h, tc, prio_tc) : -EOPNOTSUPP; + if (ret) + goto err_out; + + if (tc <= 1) { netdev_reset_tc(netdev); - return 0; + goto out; } /* Set num_tc for netdev */ ret = netdev_set_num_tc(netdev, tc); if (ret) - return ret; + goto err_out; /* Set per TC queues for the VSI */ for (i = 0; i < HNAE3_MAX_TC; i++) { @@ -1229,7 +1250,14 @@ static int hns3_setup_tc(struct net_device *netdev, u8 tc) kinfo->tc_info[i].tqp_offset); } - return 0; +out: + ret = hns3_nic_set_real_num_queue(netdev); + +err_out: + if (if_running) + (void)hns3_nic_net_open(netdev); + + return ret; } static int hns3_nic_setup_tc(struct net_device *dev, enum tc_setup_type type, @@ -1240,7 +1268,7 @@ static int hns3_nic_setup_tc(struct net_device *dev, enum tc_setup_type type, if (type != TC_SETUP_MQPRIO) 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, @@ -2848,10 +2876,69 @@ static void hns3_link_status_change(struct hnae3_handle *handle, bool linkup) } } +static int hns3_client_setup_tc(struct hnae3_handle *handle, u8 tc) +{ + struct hnae3_knic_private_info *kinfo = &handle->kinfo; + struct net_device *ndev = kinfo->netdev; + bool if_running = netif_running(ndev); + int ret; + u8 i; + + if (tc > HNAE3_MAX_TC) + return -EINVAL; + + if (!ndev) + return -ENODEV; + + ret = netdev_set_num_tc(ndev, tc); + if (ret) + return ret; + + if (if_running) { + (void)hns3_nic_net_stop(ndev); + msleep(100); + } + + ret = (kinfo->dcb_ops && kinfo->dcb_ops->map_update) ? + kinfo->dcb_ops->map_update(handle) : -EOPNOTSUPP; + if (ret) + goto err_out; + + if (tc <= 1) { + netdev_reset_tc(ndev); + goto out; + } + + for (i = 0; i < HNAE3_MAX_TC; i++) { + struct hnae3_tc_info *tc_info = &kinfo->tc_info[i]; + + if (tc_info->enable) + netdev_set_tc_queue(ndev, + tc_info->tc, + tc_info->tqp_count, + tc_info->tqp_offset); + } + + for (i = 0; i < HNAE3_MAX_USER_PRIO; i++) { + netdev_set_prio_tc_map(ndev, i, + kinfo->prio_tc[i]); + } + +out: + ret = hns3_nic_set_real_num_queue(ndev); + +err_out: + if (if_running) + (void)hns3_nic_net_open(ndev); + + return ret; +} + const struct hnae3_client_ops client_ops = { .init_instance = hns3_client_init, .uninit_instance = hns3_client_uninit, .link_status_change = hns3_link_status_change, + .setup_tc = hns3_client_setup_tc, }; /* hns3_init_module - Driver registration routine
When using tc qdisc to configure DCB parameter, dcb_ops->setup_tc is used to tell hclge_dcb module to do the setup. 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. Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> --- .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 135 +++++++++++++++++---- 1 file changed, 111 insertions(+), 24 deletions(-)