| Message ID | 1394712342-15778-2-Taiwan-albertk@realtek.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Hayes Wang ... I should have spotted this before. > /* USB_RX_EARLY_AGG */ > -#define EARLY_AGG_SUPPER 0x0e832981 > +#define EARLY_AGG_SUPER ((((rx_buf_sz - 1522) / 4) << 16) | \ > + (u32)(CONFIG_RTL8152_EARLY_AGG_TIMEOUT_SUPER <= 0 ? 85 * 125 : \ > + min(CONFIG_RTL8152_EARLY_AGG_TIMEOUT_SUPER * 125, 0xffff))) > #define EARLY_AGG_HIGH 0x0e837a12 > #define EARLY_AGG_SLOW 0x0e83ffff > > @@ -1978,7 +1980,7 @@ static void r8153_set_rx_agg(struct r8152 *tp) > ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, > RX_THR_SUPPER); > ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_EARLY_AGG, > - EARLY_AGG_SUPPER); > + EARLY_AGG_SUPER); > } else { > ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, > RX_THR_HIGH); It looks as though rx_buf_sz should be a parameter to EARLY_AGG_SUPER. David -- 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
From: David Laight <David.Laight@ACULAB.COM> Date: Thu, 13 Mar 2014 13:12:35 +0000 > From: Hayes Wang > ... > > I should have spotted this before. > >> /* USB_RX_EARLY_AGG */ >> -#define EARLY_AGG_SUPPER 0x0e832981 >> +#define EARLY_AGG_SUPER ((((rx_buf_sz - 1522) / 4) << 16) | \ >> + (u32)(CONFIG_RTL8152_EARLY_AGG_TIMEOUT_SUPER <= 0 ? 85 * 125 : \ >> + min(CONFIG_RTL8152_EARLY_AGG_TIMEOUT_SUPER * 125, 0xffff))) >> #define EARLY_AGG_HIGH 0x0e837a12 >> #define EARLY_AGG_SLOW 0x0e83ffff >> >> @@ -1978,7 +1980,7 @@ static void r8153_set_rx_agg(struct r8152 *tp) >> ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, >> RX_THR_SUPPER); >> ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_EARLY_AGG, >> - EARLY_AGG_SUPPER); >> + EARLY_AGG_SUPER); >> } else { >> ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, >> RX_THR_HIGH); > > It looks as though rx_buf_sz should be a parameter to EARLY_AGG_SUPER. And I fundamentally disagree with this being a Kconfig parameter. Make it run-time calculated _or_ settable via ethtool. Thanks. -- 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
From: David Miller [mailto:davem@davemloft.net] Sent: Friday, March 14, 2014 1:22 AM [...] > And I fundamentally disagree with this being a Kconfig parameter. > > Make it run-time calculated _or_ settable via ethtool. Excuse me. How should I make it run-time calculated without a Kconfig parameter? Should I use module_param? Best Regards, Hayes -- 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
From: hayeswang <hayeswang@realtek.com> Date: Fri, 14 Mar 2014 10:37:21 +0800 > From: David Miller [mailto:davem@davemloft.net] > Sent: Friday, March 14, 2014 1:22 AM > [...] >> And I fundamentally disagree with this being a Kconfig parameter. >> >> Make it run-time calculated _or_ settable via ethtool. > > Excuse me. How should I make it run-time calculated without a > Kconfig parameter? Should I use module_param? You run-time determine the setting based upon the negotiated link speed and traffic patterns. -- 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
From: David Miller [mailto:davem@davemloft.net] Sent: Friday, March 14, 2014 12:08 PM [...] > >> And I fundamentally disagree with this being a Kconfig parameter. > >> > >> Make it run-time calculated _or_ settable via ethtool. > > > > Excuse me. How should I make it run-time calculated without a > > Kconfig parameter? Should I use module_param? > > You run-time determine the setting based upon the negotiated link > speed and traffic patterns. It is difficult to design a algorithm which considers the hardware of the platform, network traffic, and even the USB behavior to dynamically modify the setting. I don't think I have the capability to do it. Besides, I don't wish to modify the setting by ethtool when re-loading the driver or rebooting every time. Excuse me. Why is it not accepted for being a Kconfig parameter. It let the manufactuers of some platforms, especially the embedded system, could tune their performance. Should I give up this patch? -- 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
From: hayeswang <hayeswang@realtek.com> Date: Fri, 14 Mar 2014 15:24:50 +0800 > Besides, I don't wish to modify the setting by ethtool when re-loading > the driver or rebooting every time. You have code to reset the driver, you can do it when the user asks for the setting to be changed via ethtool. I do not see this as a problem. The ethtool change can occur while the driver is already up, you'll just need to reset the chip and make the new configuration, this is not a problem. > Excuse me. Why is it not accepted for being a Kconfig parameter. > It let the manufactuers of some platforms, especially the embedded > system, could tune their performance. Should I give up this patch? We want to discourage such hard-coding, not encourage it. I think you are limiting yourself unnecessarily by refusing to consider that there may be ways other than Kconfig to adjust this setting. -- 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
hayeswang <hayeswang@realtek.com> : [...] > Besides, I don't wish to modify the setting by ethtool when re-loading > the driver or rebooting every time. Why ? The recipe is different but there isn't much setup difference between a module param and an ethtool command that is run through udev. The latter is more versatile though. > Excuse me. Why is it not accepted for being a Kconfig parameter. You have stated that the optimal value is not easy to figure. It's thus hard to give much credit to an hardcoded solution.
From: David Miller [mailto:davem@davemloft.net] > Sent: Saturday, March 15, 2014 2:43 AM [...] > > Besides, I don't wish to modify the setting by ethtool when re-loading > > the driver or rebooting every time. > > You have code to reset the driver, you can do it when the user asks > for the setting to be changed via ethtool. I do not see this as > a problem. > > The ethtool change can occur while the driver is already up, you'll > just need to reset the chip and make the new configuration, this is > not a problem. Thanks for your answer. I would study how to set it by ethtool. Best Regards, Hayes -- 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
From: Francois Romieu [mailto:romieu@fr.zoreil.com] > Sent: Saturday, March 15, 2014 7:43 AM [...] > > Besides, I don't wish to modify the setting by ethtool when re-loading > > the driver or rebooting every time. > > Why ? > > The recipe is different but there isn't much setup difference > between a module param and an ethtool command that is run through udev. > The latter is more versatile though. Thanks for your suggestion. I think I have to understand how to use them first. Best Regards, Hayes -- 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
diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig index 7e7269f..a8639b8 100644 --- a/drivers/net/usb/Kconfig +++ b/drivers/net/usb/Kconfig @@ -102,6 +102,18 @@ config USB_RTL8152 To compile this driver as a module, choose M here: the module will be called r8152. + menu "Aggregation Settings" + depends on USB_RTL8152 + + config RTL8152_EARLY_AGG_TIMEOUT_SUPER + int "rx early agg timeout for super speed (unit: us)" + default 85 + help + This is the rx early agg timeout for USB super speed. + The vaild value is 1 ~ 525 us. + + endmenu + config USB_USBNET tristate "Multi-purpose USB Networking Framework" select MII diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index aa1d5b2..756c0a6 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -316,7 +316,9 @@ #define PCUT_STATUS 0x0001 /* USB_RX_EARLY_AGG */ -#define EARLY_AGG_SUPPER 0x0e832981 +#define EARLY_AGG_SUPER ((((rx_buf_sz - 1522) / 4) << 16) | \ + (u32)(CONFIG_RTL8152_EARLY_AGG_TIMEOUT_SUPER <= 0 ? 85 * 125 : \ + min(CONFIG_RTL8152_EARLY_AGG_TIMEOUT_SUPER * 125, 0xffff))) #define EARLY_AGG_HIGH 0x0e837a12 #define EARLY_AGG_SLOW 0x0e83ffff @@ -1978,7 +1980,7 @@ static void r8153_set_rx_agg(struct r8152 *tp) ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, RX_THR_SUPPER); ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_EARLY_AGG, - EARLY_AGG_SUPPER); + EARLY_AGG_SUPER); } else { ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, RX_THR_HIGH);
For slow CPU, the frequent bulk transfer would cause poor throughput. One solution is to increase the timeout of the aggregation. It let the hw could complete the bulk transfer later and fill more packets into the buffer. Besides, it could reduce the frequency of the bulk transfer efficiently and improve the performance. However, the optimization value of the timeout depends on the capability of the hardware, especially the CPU. For example, according to the experiment, the timeout 164 us is better than the default value for the chromebook with the ARM CPU. Now add RTL8152_EARLY_AGG_TIMEOUT_SUPER to let someone could choose desired timeout value if he wants to get the best performance. Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/usb/Kconfig | 12 ++++++++++++ drivers/net/usb/r8152.c | 6 ++++-- 2 files changed, 16 insertions(+), 2 deletions(-)