| Message ID | 20240608044557.1380550-1-xiaolei.wang@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net,v4] net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters | expand |
On Sat, Jun 08, 2024 at 12:45:57PM +0800, Xiaolei Wang wrote: > The current cbs parameter depends on speed after uplinking, > which is not needed and will report a configuration error > if the port is not initially connected. The UAPI exposed by > tc-cbs requires userspace to recalculate the send slope anyway, > because the formula depends on port_transmit_rate (see man tc-cbs), > which is not an invariant from tc's perspective. Therefore, we > use offload->sendslope and offload->idleslope to derive the > original port_transmit_rate from the CBS formula. > > Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC") > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > --- > > Change log: > > v1: > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240528092010.439089-1-xiaolei.wang@windriver.com/ > v2: > Update CBS parameters when speed changes after linking up > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240530061453.561708-1-xiaolei.wang@windriver.com/ > v3: > replace priv->speed with the portTransmitRate from the tc-cbs parameters suggested by Vladimir Oltean > link: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240607103327.438455-1-xiaolei.wang@windriver.com/ > v4: > Delete speed_div variable, delete redundant port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope; and update commit log > > .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 20 +++++++------------ > 1 file changed, 7 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > index 222540b55480..87af129a6a1d 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > @@ -344,10 +344,11 @@ static int tc_setup_cbs(struct stmmac_priv *priv, > { > u32 tx_queues_count = priv->plat->tx_queues_to_use; > u32 queue = qopt->queue; > - u32 ptr, speed_div; > + u32 ptr; > u32 mode_to_use; > u64 value; > int ret; > + s64 port_transmit_rate_kbps; The feedback that came along with Wojciech's review in v3 was to use reverse Christmas tree (RCT) variable ordering. That means to sort variable declarations from longest line to shortest. It is the de facto coding style standard for kernel networking code. pw-bot: changes-requested
On 6/8/24 19:16, Vladimir Oltean wrote: > CAUTION: This email comes from a non Wind River email account! > Do not click links or open attachments unless you recognize the sender and know the content is safe. > > On Sat, Jun 08, 2024 at 12:45:57PM +0800, Xiaolei Wang wrote: >> The current cbs parameter depends on speed after uplinking, >> which is not needed and will report a configuration error >> if the port is not initially connected. The UAPI exposed by >> tc-cbs requires userspace to recalculate the send slope anyway, >> because the formula depends on port_transmit_rate (see man tc-cbs), >> which is not an invariant from tc's perspective. Therefore, we >> use offload->sendslope and offload->idleslope to derive the >> original port_transmit_rate from the CBS formula. >> >> Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC") >> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> >> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >> --- >> >> Change log: >> >> v1: >> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240528092010.439089-1-xiaolei.wang@windriver.com/ >> v2: >> Update CBS parameters when speed changes after linking up >> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240530061453.561708-1-xiaolei.wang@windriver.com/ >> v3: >> replace priv->speed with the portTransmitRate from the tc-cbs parameters suggested by Vladimir Oltean >> link: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240607103327.438455-1-xiaolei.wang@windriver.com/ >> v4: >> Delete speed_div variable, delete redundant port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope; and update commit log >> >> .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 20 +++++++------------ >> 1 file changed, 7 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c >> index 222540b55480..87af129a6a1d 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c >> @@ -344,10 +344,11 @@ static int tc_setup_cbs(struct stmmac_priv *priv, >> { >> u32 tx_queues_count = priv->plat->tx_queues_to_use; >> u32 queue = qopt->queue; >> - u32 ptr, speed_div; >> + u32 ptr; >> u32 mode_to_use; >> u64 value; >> int ret; >> + s64 port_transmit_rate_kbps; > The feedback that came along with Wojciech's review in v3 was to use > reverse Christmas tree (RCT) variable ordering. That means to sort > variable declarations from longest line to shortest. It is the de facto > coding style standard for kernel networking code. I will send a new version thanks xiaolei > > pw-bot: changes-requested
On Sat, Jun 08, 2024 at 12:45:57PM +0800, Xiaolei Wang wrote: > The current cbs parameter depends on speed after uplinking, > which is not needed and will report a configuration error > if the port is not initially connected. The UAPI exposed by > tc-cbs requires userspace to recalculate the send slope anyway, > because the formula depends on port_transmit_rate (see man tc-cbs), > which is not an invariant from tc's perspective. Therefore, we > use offload->sendslope and offload->idleslope to derive the > original port_transmit_rate from the CBS formula. > > Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC") > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > --- > > Change log: > > v1: > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240528092010.439089-1-xiaolei.wang@windriver.com/ > v2: > Update CBS parameters when speed changes after linking up > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240530061453.561708-1-xiaolei.wang@windriver.com/ > v3: > replace priv->speed with the portTransmitRate from the tc-cbs parameters suggested by Vladimir Oltean > link: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240607103327.438455-1-xiaolei.wang@windriver.com/ > v4: > Delete speed_div variable, delete redundant port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope; and update commit log > > .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 20 +++++++------------ > 1 file changed, 7 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > index 222540b55480..87af129a6a1d 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > @@ -344,10 +344,11 @@ static int tc_setup_cbs(struct stmmac_priv *priv, > { > u32 tx_queues_count = priv->plat->tx_queues_to_use; > u32 queue = qopt->queue; > - u32 ptr, speed_div; > + u32 ptr; > u32 mode_to_use; > u64 value; > int ret; > + s64 port_transmit_rate_kbps; > > /* Queue 0 is not AVB capable */ > if (queue <= 0 || queue >= tx_queues_count) > @@ -355,27 +356,20 @@ static int tc_setup_cbs(struct stmmac_priv *priv, > if (!priv->dma_cap.av) > return -EOPNOTSUPP; > > + port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope; > + > /* Port Transmit Rate and Speed Divider */ > - switch (priv->speed) { > + switch (div_s64(port_transmit_rate_kbps, 1000)) { > case SPEED_10000: > - ptr = 32; > - speed_div = 10000000; > - break; > case SPEED_5000: > ptr = 32; > - speed_div = 5000000; > break; > case SPEED_2500: > - ptr = 8; > - speed_div = 2500000; > - break; > case SPEED_1000: > ptr = 8; > - speed_div = 1000000; > break; > case SPEED_100: > ptr = 4; > - speed_div = 100000; > break; > default: > return -EOPNOTSUPP; I have one more request. It is very discouraging for a user to give invalid parameters and receive -EOPNOTSUPP, because this is indicative of other "usual suspects": "did I enable CONFIG_NET_SCH_CBS?" It is unfortunate that struct tc_cbs_qopt_offload does not carry a netlink extack, but I'm also not requesting you to add one here. Instead, please at least change the return code to something like -EINVAL, and print something to the console along the lines of: "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n".
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c index 222540b55480..87af129a6a1d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c @@ -344,10 +344,11 @@ static int tc_setup_cbs(struct stmmac_priv *priv, { u32 tx_queues_count = priv->plat->tx_queues_to_use; u32 queue = qopt->queue; - u32 ptr, speed_div; + u32 ptr; u32 mode_to_use; u64 value; int ret; + s64 port_transmit_rate_kbps; /* Queue 0 is not AVB capable */ if (queue <= 0 || queue >= tx_queues_count) @@ -355,27 +356,20 @@ static int tc_setup_cbs(struct stmmac_priv *priv, if (!priv->dma_cap.av) return -EOPNOTSUPP; + port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope; + /* Port Transmit Rate and Speed Divider */ - switch (priv->speed) { + switch (div_s64(port_transmit_rate_kbps, 1000)) { case SPEED_10000: - ptr = 32; - speed_div = 10000000; - break; case SPEED_5000: ptr = 32; - speed_div = 5000000; break; case SPEED_2500: - ptr = 8; - speed_div = 2500000; - break; case SPEED_1000: ptr = 8; - speed_div = 1000000; break; case SPEED_100: ptr = 4; - speed_div = 100000; break; default: return -EOPNOTSUPP; @@ -398,10 +392,10 @@ static int tc_setup_cbs(struct stmmac_priv *priv, } /* Final adjustments for HW */ - value = div_s64(qopt->idleslope * 1024ll * ptr, speed_div); + value = div_s64(qopt->idleslope * 1024ll * ptr, port_transmit_rate_kbps); priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0); - value = div_s64(-qopt->sendslope * 1024ll * ptr, speed_div); + value = div_s64(-qopt->sendslope * 1024ll * ptr, port_transmit_rate_kbps); priv->plat->tx_queues_cfg[queue].send_slope = value & GENMASK(31, 0); value = qopt->hicredit * 1024ll * 8;
The current cbs parameter depends on speed after uplinking, which is not needed and will report a configuration error if the port is not initially connected. The UAPI exposed by tc-cbs requires userspace to recalculate the send slope anyway, because the formula depends on port_transmit_rate (see man tc-cbs), which is not an invariant from tc's perspective. Therefore, we use offload->sendslope and offload->idleslope to derive the original port_transmit_rate from the CBS formula. Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC") Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> --- Change log: v1: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240528092010.439089-1-xiaolei.wang@windriver.com/ v2: Update CBS parameters when speed changes after linking up https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240530061453.561708-1-xiaolei.wang@windriver.com/ v3: replace priv->speed with the portTransmitRate from the tc-cbs parameters suggested by Vladimir Oltean link: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240607103327.438455-1-xiaolei.wang@windriver.com/ v4: Delete speed_div variable, delete redundant port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope; and update commit log .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-)