| Message ID | 1425473468-25969-2-git-send-email-ogerlitz@mellanox.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 03/04/2015 04:51 AM, Or Gerlitz wrote: > From: Shani Michaeli <shanim@mellanox.com> > > As specified in 802.1Qau spec. Add this optional attribute to the > DCB netlink layer. To allow for application to use the new attribute, > NIC drivers should implement and register the callbacks ieee_getqcn, > ieee_setqcn and ieee_getqcnstats. > > The QCN attribute holds a set of parameters for management, and > a set of statistics to provide informative data on Congestion-Control > defined by this spec. > > Signed-off-by: Shani Michaeli <shanim@mellanox.com> > Signed-off-by: Shachar Raindel <raindel@mellanox.com> > Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com> > --- Looks good to me. Do you have a QCN enabled switch? I looked at implementing this awhile ago but didn't have any switch support so I never did it. Also do you have a user space client to configure this? I would like it if someone wanted to add support to lldpad/dcbtool. [...] > + > +/* This structure contains the IEEE 802.1Qau QCN managed object. > + * > + *@rpg_enable: enable QCN RP > + *@rppp_max_rps: maximum number of RPs allowed for this CNPV on this port > + *@rpg_time_reset: time between rate increases if no CNMs received. > + * given in u-seconds > + *@rpg_byte_reset: transmitted data between rate increases if no CNMs received. > + * given in Bytes > + *@rpg_threshold: The number of times rpByteStage or rpTimeStage can count > + * before RP rate control state machine advances states > + *@rpg_max_rate: the maxinun rate, in Mbits per second, > + * at which an RP can transmit > + *@rpg_ai_rate: The rate, in Mbits per second, > + * used to increase rpTargetRate in the RPR_ACTIVE_INCREASE > + *@rpg_hai_rate: The rate, in Mbits per second, > + * used to increase rpTargetRate in the RPR_HYPER_INCREASE state > + *@rpg_gd: Upon CNM receive, flow rate is limited to (Fb/Gd)*CurrentRate. > + * rpgGd is given as log2(Gd), where Gd may only be powers of 2 > + *@rpg_min_dec_fac: The minimum factor by which the current transmit rate > + * can be changed by reception of a CNM. > + * value is given as percentage (1-100) > + *@rpg_min_rate: The minimum value, in bits per second, for rate to limit > + *@cndd_state_machine: The state of the congestion notification domain > + * defense state machine, as defined by IEEE 802.3Qau > + * section 32.1.1. In the interior ready state, > + * the QCN capable hardware may add CN-TAG TLV to the > + * outgoing traffic, to specifically identify outgoing > + * flows. > + */ I'm assuming this structure maps to an IEEE MIB? Its a rather large structure for a single netlink type but this seems to be how we built the dcbnl interface and if it does seem logical that the structure is one logical block, meaning you need to supply all fields. [...] > > + if (ops->ieee_getqcn) { > + struct ieee_qcn qcn; you might consider adding a newline here it is the best practice for new code although dcbnl has plenty of examples where it doesn't use this convention. > + memset(&qcn, 0, sizeof(qcn)); > + err = ops->ieee_getqcn(netdev, &qcn); > + if (!err) { > + err = nla_put(skb, DCB_ATTR_IEEE_QCN, > + sizeof(qcn), &qcn); > + if (err) > + return -EMSGSIZE; > + } > + } > + > + if (ops->ieee_getqcnstats) { > + struct ieee_qcn_stats qcn_stats; same here. > + memset(&qcn_stats, 0, sizeof(qcn_stats)); > + err = ops->ieee_getqcnstats(netdev, &qcn_stats); > + if (!err) { > + err = nla_put(skb, DCB_ATTR_IEEE_QCN_STATS, > + sizeof(qcn_stats), &qcn_stats); > + if (err) > + return -EMSGSIZE; > + } > + } > + > if (ops->ieee_getpfc) { > struct ieee_pfc pfc; > memset(&pfc, 0, sizeof(pfc)); > @@ -1379,8 +1405,9 @@ int dcbnl_cee_notify(struct net_device *dev, int event, int cmd, > } > EXPORT_SYMBOL(dcbnl_cee_notify); > > -/* Handle IEEE 802.1Qaz SET commands. If any requested operation can not > - * be completed the entire msg is aborted and error value is returned. > +/* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb SET commands. > + * If any requested operation can not be completed > + * the entire msg is aborted and error value is returned. > * No attempt is made to reconcile the case where only part of the > * cmd can be completed. > */ > @@ -1417,6 +1444,14 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh, > goto err; > } > > + if (ieee[DCB_ATTR_IEEE_QCN] && ops->ieee_setqcn) { > + struct ieee_qcn *qcn = > + nla_data(ieee[DCB_ATTR_IEEE_QCN]); same here. > + err = ops->ieee_setqcn(netdev, qcn); > + if (err) > + goto err; > + } > + [...] Feel free to add my acked by if you respin it with the newlines. Acked-by: John Fastabend <john.r.fastabend@intel.com>
On Wed, Mar 4, 2015 at 7:19 PM, John Fastabend <john.fastabend@gmail.com> wrote: > On 03/04/2015 04:51 AM, Or Gerlitz wrote: >> >> From: Shani Michaeli <shanim@mellanox.com> >> >> As specified in 802.1Qau spec. Add this optional attribute to the >> DCB netlink layer. To allow for application to use the new attribute, >> NIC drivers should implement and register the callbacks ieee_getqcn, >> ieee_setqcn and ieee_getqcnstats. >> >> The QCN attribute holds a set of parameters for management, and >> a set of statistics to provide informative data on Congestion-Control >> defined by this spec. >> >> Signed-off-by: Shani Michaeli <shanim@mellanox.com> >> Signed-off-by: Shachar Raindel <raindel@mellanox.com> >> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com> > Looks good to me. Do you have a QCN enabled switch? I looked at > implementing this awhile ago but didn't have any switch support so > I never did it. I'l let Shachar to address the testing and the MIB questions. > Also do you have a user space client to configure this? I would like > it if someone wanted to add support to lldpad/dcbtool. Sure, we have some netlink (python scripts) code to configure/read this towards the kernel. > [...] >> + >> +/* This structure contains the IEEE 802.1Qau QCN managed object. >> + * >> + *@rpg_enable: enable QCN RP >> + *@rppp_max_rps: maximum number of RPs allowed for this CNPV on this port >> + *@rpg_time_reset: time between rate increases if no CNMs received. >> + * given in u-seconds >> + *@rpg_byte_reset: transmitted data between rate increases if no CNMs >> received. >> + * given in Bytes >> + *@rpg_threshold: The number of times rpByteStage or rpTimeStage can >> count >> + * before RP rate control state machine advances states >> + *@rpg_max_rate: the maxinun rate, in Mbits per second, >> + * at which an RP can transmit >> + *@rpg_ai_rate: The rate, in Mbits per second, >> + * used to increase rpTargetRate in the RPR_ACTIVE_INCREASE >> + *@rpg_hai_rate: The rate, in Mbits per second, >> + * used to increase rpTargetRate in the RPR_HYPER_INCREASE >> state >> + *@rpg_gd: Upon CNM receive, flow rate is limited to (Fb/Gd)*CurrentRate. >> + * rpgGd is given as log2(Gd), where Gd may only be powers of 2 >> + *@rpg_min_dec_fac: The minimum factor by which the current transmit rate >> + * can be changed by reception of a CNM. >> + * value is given as percentage (1-100) >> + *@rpg_min_rate: The minimum value, in bits per second, for rate to limit >> + *@cndd_state_machine: The state of the congestion notification domain >> + * defense state machine, as defined by IEEE 802.3Qau >> + * section 32.1.1. In the interior ready state, >> + * the QCN capable hardware may add CN-TAG TLV to the >> + * outgoing traffic, to specifically identify outgoing >> + * flows. >> + */ > > > I'm assuming this structure maps to an IEEE MIB? yep, I guess so > Its a rather large structure for a single netlink type but this seems > to be how we built > the dcbnl interface and if it does seem logical that the structure is > one logical block, meaning you need to supply all fields. > [...] >> + if (ops->ieee_getqcn) { >> + struct ieee_qcn qcn; > you might consider adding a newline here it is the best practice for > new code although dcbnl has plenty of examples where it doesn't use > this convention. OK, will add this here and in the other places where used that practice. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Or Gerlitz [mailto:gerlitz.or@gmail.com] > Sent: Thursday, March 05, 2015 8:54 AM > To: John Fastabend > Cc: Or Gerlitz; David S. Miller; John Fastabend; Linux Netdev List; Amir > Vadai; Tal Alon; Shani Michaeli; Shachar Raindel > Subject: Re: [PATCH net-next 1/3] net/dcb: Add IEEE QCN attribute > > On Wed, Mar 4, 2015 at 7:19 PM, John Fastabend > <john.fastabend@gmail.com> wrote: > > On 03/04/2015 04:51 AM, Or Gerlitz wrote: > >> > >> From: Shani Michaeli <shanim@mellanox.com> > >> > >> As specified in 802.1Qau spec. Add this optional attribute to the > >> DCB netlink layer. To allow for application to use the new attribute, > >> NIC drivers should implement and register the callbacks ieee_getqcn, > >> ieee_setqcn and ieee_getqcnstats. > >> > >> The QCN attribute holds a set of parameters for management, and > >> a set of statistics to provide informative data on Congestion-Control > >> defined by this spec. > >> > >> Signed-off-by: Shani Michaeli <shanim@mellanox.com> > >> Signed-off-by: Shachar Raindel <raindel@mellanox.com> > >> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com> > > > Looks good to me. Do you have a QCN enabled switch? I looked at > > implementing this awhile ago but didn't have any switch support so > > I never did it. > > I'l let Shachar to address the testing and the MIB questions. The Mellanox SwitchX-2 IC supports QCN. We were testing our NICs both with this switch IC and using internal test fixtures where we were injecting congestion notification messages as raw Ethernet packets by another host. > > > Also do you have a user space client to configure this? I would like > > it if someone wanted to add support to lldpad/dcbtool. > > Sure, we have some netlink (python scripts) code to configure/read > this towards the kernel. > > > > [...] > >> + > >> +/* This structure contains the IEEE 802.1Qau QCN managed object. > >> + * > >> + *@rpg_enable: enable QCN RP > >> + *@rppp_max_rps: maximum number of RPs allowed for this CNPV on this > port > >> + *@rpg_time_reset: time between rate increases if no CNMs received. > >> + * given in u-seconds > >> + *@rpg_byte_reset: transmitted data between rate increases if no > CNMs > >> received. > >> + * given in Bytes > >> + *@rpg_threshold: The number of times rpByteStage or rpTimeStage can > >> count > >> + * before RP rate control state machine advances > states > >> + *@rpg_max_rate: the maxinun rate, in Mbits per second, > >> + * at which an RP can transmit > >> + *@rpg_ai_rate: The rate, in Mbits per second, > >> + * used to increase rpTargetRate in the > RPR_ACTIVE_INCREASE > >> + *@rpg_hai_rate: The rate, in Mbits per second, > >> + * used to increase rpTargetRate in the > RPR_HYPER_INCREASE > >> state > >> + *@rpg_gd: Upon CNM receive, flow rate is limited to > (Fb/Gd)*CurrentRate. > >> + * rpgGd is given as log2(Gd), where Gd may only be powers of > 2 > >> + *@rpg_min_dec_fac: The minimum factor by which the current transmit > rate > >> + * can be changed by reception of a CNM. > >> + * value is given as percentage (1-100) > >> + *@rpg_min_rate: The minimum value, in bits per second, for rate to > limit > >> + *@cndd_state_machine: The state of the congestion notification > domain > >> + * defense state machine, as defined by IEEE > 802.3Qau > >> + * section 32.1.1. In the interior ready state, > >> + * the QCN capable hardware may add CN-TAG TLV to > the > >> + * outgoing traffic, to specifically identify > outgoing > >> + * flows. > >> + */ > > > > > > I'm assuming this structure maps to an IEEE MIB? > > yep, I guess so The structures map to the IEEE MIB for QCN (part of IEEE 802.1Qau). The fields rppp_max_rps and cndd_state_machine are in different sections than the rest of the fields. However, it seems bit redundant to define a whole struct just for one field. The cndd_state_machine is not explicitly defined in the MIB, as the LLDP negotiation, which the standard assumes, is implemented by lldpad. > > > Its a rather large structure for a single netlink type but this seems > > to be how we built > > the dcbnl interface and if it does seem logical that the structure is > > one logical block, meaning you need to supply all fields. > This is the set of parameters defining how you will reduce or increase you TX rate upon receiving CNM from the network. I agree that it is a long list, but this is how the standard was written... > > [...] > >> + if (ops->ieee_getqcn) { > >> + struct ieee_qcn qcn; > > > you might consider adding a newline here it is the best practice for > > new code although dcbnl has plenty of examples where it doesn't use > > this convention. > > OK, will add this here and in the other places where used that practice.
[...] >> >>> Looks good to me. Do you have a QCN enabled switch? I looked at >>> implementing this awhile ago but didn't have any switch support so >>> I never did it. >> >> I'l let Shachar to address the testing and the MIB questions. > > The Mellanox SwitchX-2 IC supports QCN. We were testing our NICs both > with this switch IC and using internal test fixtures where we were > injecting congestion notification messages as raw Ethernet packets by > another host. ah great didn't know that switch supported QCN. > >> >>> Also do you have a user space client to configure this? I would like >>> it if someone wanted to add support to lldpad/dcbtool. >> >> Sure, we have some netlink (python scripts) code to configure/read >> this towards the kernel. >> >> >>> [...] [...] >>> >>> >>> I'm assuming this structure maps to an IEEE MIB? >> >> yep, I guess so > > The structures map to the IEEE MIB for QCN (part of IEEE 802.1Qau). > The fields rppp_max_rps and cndd_state_machine are in different sections > than the rest of the fields. However, it seems bit redundant to define a > whole struct just for one field. The cndd_state_machine is not explicitly > defined in the MIB, as the LLDP negotiation, which the standard assumes, > is implemented by lldpad. > >> >>> Its a rather large structure for a single netlink type but this seems >>> to be how we built >>> the dcbnl interface and if it does seem logical that the structure is >>> one logical block, meaning you need to supply all fields. >> > > This is the set of parameters defining how you will reduce or increase you > TX rate upon receiving CNM from the network. I agree that it is a long list, > but this is how the standard was written... yep works for me. I was just checking my assumptions are correct and admittedly being a bit lazy so I didn't pull up the spec myself. Thanks, John [...]
diff --git a/include/net/dcbnl.h b/include/net/dcbnl.h index 597b88a..207d9ba 100644 --- a/include/net/dcbnl.h +++ b/include/net/dcbnl.h @@ -49,6 +49,9 @@ struct dcbnl_rtnl_ops { int (*ieee_setets) (struct net_device *, struct ieee_ets *); int (*ieee_getmaxrate) (struct net_device *, struct ieee_maxrate *); int (*ieee_setmaxrate) (struct net_device *, struct ieee_maxrate *); + int (*ieee_getqcn) (struct net_device *, struct ieee_qcn *); + int (*ieee_setqcn) (struct net_device *, struct ieee_qcn *); + int (*ieee_getqcnstats) (struct net_device *, struct ieee_qcn_stats *); int (*ieee_getpfc) (struct net_device *, struct ieee_pfc *); int (*ieee_setpfc) (struct net_device *, struct ieee_pfc *); int (*ieee_getapp) (struct net_device *, struct dcb_app *); diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h index e711f20..6497d79 100644 --- a/include/uapi/linux/dcbnl.h +++ b/include/uapi/linux/dcbnl.h @@ -78,6 +78,70 @@ struct ieee_maxrate { __u64 tc_maxrate[IEEE_8021QAZ_MAX_TCS]; }; +enum dcbnl_cndd_states { + DCB_CNDD_RESET = 0, + DCB_CNDD_EDGE, + DCB_CNDD_INTERIOR, + DCB_CNDD_INTERIOR_READY, +}; + +/* This structure contains the IEEE 802.1Qau QCN managed object. + * + *@rpg_enable: enable QCN RP + *@rppp_max_rps: maximum number of RPs allowed for this CNPV on this port + *@rpg_time_reset: time between rate increases if no CNMs received. + * given in u-seconds + *@rpg_byte_reset: transmitted data between rate increases if no CNMs received. + * given in Bytes + *@rpg_threshold: The number of times rpByteStage or rpTimeStage can count + * before RP rate control state machine advances states + *@rpg_max_rate: the maxinun rate, in Mbits per second, + * at which an RP can transmit + *@rpg_ai_rate: The rate, in Mbits per second, + * used to increase rpTargetRate in the RPR_ACTIVE_INCREASE + *@rpg_hai_rate: The rate, in Mbits per second, + * used to increase rpTargetRate in the RPR_HYPER_INCREASE state + *@rpg_gd: Upon CNM receive, flow rate is limited to (Fb/Gd)*CurrentRate. + * rpgGd is given as log2(Gd), where Gd may only be powers of 2 + *@rpg_min_dec_fac: The minimum factor by which the current transmit rate + * can be changed by reception of a CNM. + * value is given as percentage (1-100) + *@rpg_min_rate: The minimum value, in bits per second, for rate to limit + *@cndd_state_machine: The state of the congestion notification domain + * defense state machine, as defined by IEEE 802.3Qau + * section 32.1.1. In the interior ready state, + * the QCN capable hardware may add CN-TAG TLV to the + * outgoing traffic, to specifically identify outgoing + * flows. + */ + +struct ieee_qcn { + __u8 rpg_enable[IEEE_8021QAZ_MAX_TCS]; + __u32 rppp_max_rps[IEEE_8021QAZ_MAX_TCS]; + __u32 rpg_time_reset[IEEE_8021QAZ_MAX_TCS]; + __u32 rpg_byte_reset[IEEE_8021QAZ_MAX_TCS]; + __u32 rpg_threshold[IEEE_8021QAZ_MAX_TCS]; + __u32 rpg_max_rate[IEEE_8021QAZ_MAX_TCS]; + __u32 rpg_ai_rate[IEEE_8021QAZ_MAX_TCS]; + __u32 rpg_hai_rate[IEEE_8021QAZ_MAX_TCS]; + __u32 rpg_gd[IEEE_8021QAZ_MAX_TCS]; + __u32 rpg_min_dec_fac[IEEE_8021QAZ_MAX_TCS]; + __u32 rpg_min_rate[IEEE_8021QAZ_MAX_TCS]; + __u32 cndd_state_machine[IEEE_8021QAZ_MAX_TCS]; +}; + +/* This structure contains the IEEE 802.1Qau QCN statistics. + * + *@rppp_rp_centiseconds: the number of RP-centiseconds accumulated + * by RPs at this priority level on this Port + *@rppp_created_rps: number of active RPs(flows) that react to CNMs + */ + +struct ieee_qcn_stats { + __u64 rppp_rp_centiseconds[IEEE_8021QAZ_MAX_TCS]; + __u32 rppp_created_rps[IEEE_8021QAZ_MAX_TCS]; +}; + /* This structure contains the IEEE 802.1Qaz PFC managed object * * @pfc_cap: Indicates the number of traffic classes on the local device @@ -334,6 +398,8 @@ enum ieee_attrs { DCB_ATTR_IEEE_PEER_PFC, DCB_ATTR_IEEE_PEER_APP, DCB_ATTR_IEEE_MAXRATE, + DCB_ATTR_IEEE_QCN, + DCB_ATTR_IEEE_QCN_STATS, __DCB_ATTR_IEEE_MAX }; #define DCB_ATTR_IEEE_MAX (__DCB_ATTR_IEEE_MAX - 1) diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c index 93ea801..6fce458 100644 --- a/net/dcb/dcbnl.c +++ b/net/dcb/dcbnl.c @@ -177,6 +177,8 @@ static const struct nla_policy dcbnl_ieee_policy[DCB_ATTR_IEEE_MAX + 1] = { [DCB_ATTR_IEEE_PFC] = {.len = sizeof(struct ieee_pfc)}, [DCB_ATTR_IEEE_APP_TABLE] = {.type = NLA_NESTED}, [DCB_ATTR_IEEE_MAXRATE] = {.len = sizeof(struct ieee_maxrate)}, + [DCB_ATTR_IEEE_QCN] = {.len = sizeof(struct ieee_qcn)}, + [DCB_ATTR_IEEE_QCN_STATS] = {.len = sizeof(struct ieee_qcn_stats)}, }; static const struct nla_policy dcbnl_ieee_app[DCB_ATTR_IEEE_APP_MAX + 1] = { @@ -1030,7 +1032,7 @@ nla_put_failure: return err; } -/* Handle IEEE 802.1Qaz GET commands. */ +/* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb GET commands. */ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev) { struct nlattr *ieee, *app; @@ -1067,6 +1069,30 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev) } } + if (ops->ieee_getqcn) { + struct ieee_qcn qcn; + memset(&qcn, 0, sizeof(qcn)); + err = ops->ieee_getqcn(netdev, &qcn); + if (!err) { + err = nla_put(skb, DCB_ATTR_IEEE_QCN, + sizeof(qcn), &qcn); + if (err) + return -EMSGSIZE; + } + } + + if (ops->ieee_getqcnstats) { + struct ieee_qcn_stats qcn_stats; + memset(&qcn_stats, 0, sizeof(qcn_stats)); + err = ops->ieee_getqcnstats(netdev, &qcn_stats); + if (!err) { + err = nla_put(skb, DCB_ATTR_IEEE_QCN_STATS, + sizeof(qcn_stats), &qcn_stats); + if (err) + return -EMSGSIZE; + } + } + if (ops->ieee_getpfc) { struct ieee_pfc pfc; memset(&pfc, 0, sizeof(pfc)); @@ -1379,8 +1405,9 @@ int dcbnl_cee_notify(struct net_device *dev, int event, int cmd, } EXPORT_SYMBOL(dcbnl_cee_notify); -/* Handle IEEE 802.1Qaz SET commands. If any requested operation can not - * be completed the entire msg is aborted and error value is returned. +/* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb SET commands. + * If any requested operation can not be completed + * the entire msg is aborted and error value is returned. * No attempt is made to reconcile the case where only part of the * cmd can be completed. */ @@ -1417,6 +1444,14 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh, goto err; } + if (ieee[DCB_ATTR_IEEE_QCN] && ops->ieee_setqcn) { + struct ieee_qcn *qcn = + nla_data(ieee[DCB_ATTR_IEEE_QCN]); + err = ops->ieee_setqcn(netdev, qcn); + if (err) + goto err; + } + if (ieee[DCB_ATTR_IEEE_PFC] && ops->ieee_setpfc) { struct ieee_pfc *pfc = nla_data(ieee[DCB_ATTR_IEEE_PFC]); err = ops->ieee_setpfc(netdev, pfc);