| Message ID | 1446689411-22263-4-git-send-email-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Nov 04, 2015 at 09:10:11PM -0500, ira.weiny@intel.com wrote: > From: Vennila Megavannan <vennila.megavannan@intel.com> > > Add a module paramter to toggle prescan/Fast ECN Detection. > > In addition change the PRESCAN_RXQ Kconfig default to "yes". > > Reviewed-by: Arthur Kepner<arthur.kepner@intel.com> > Reviewed-by: Mike Marciniszyn<mike.marciniszyn@intel.com> > Signed-off-by: Vennila Megavannan<vennila.megavannan@intel.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Hm... In the original code we had the config entry but the code to support this was actually disabled. Now we are turning on the config entry by default but the module parameter defaults to disabled. I think the documentation is a bit tricky because it should say that actually enabling the config is not enough, it's still disabled. Do we really need the config to be there? Distros are going to enable it. Who is it who wants to disable the config? Also is it better to default to off or on for this code, what are the upsides and downsides of that choice? regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 05, 2015 at 10:56:27AM +0300, Dan Carpenter wrote: > On Wed, Nov 04, 2015 at 09:10:11PM -0500, ira.weiny@intel.com wrote: > > From: Vennila Megavannan <vennila.megavannan@intel.com> > > > > Add a module paramter to toggle prescan/Fast ECN Detection. > > > > In addition change the PRESCAN_RXQ Kconfig default to "yes". > > > > Reviewed-by: Arthur Kepner<arthur.kepner@intel.com> > > Reviewed-by: Mike Marciniszyn<mike.marciniszyn@intel.com> > > Signed-off-by: Vennila Megavannan<vennila.megavannan@intel.com> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > Hm... In the original code we had the config entry but the code to > support this was actually disabled. Now we are turning on the config > entry by default but the module parameter defaults to disabled. I think > the documentation is a bit tricky because it should say that actually > enabling the config is not enough, it's still disabled. We tried to make it clear but obviously missed the mark. > > Do we really need the config to be there? Distros are going to enable it. > Who is it who wants to disable the config? The config is maintained to be able to fully remove the code to obtain the maximum receive performance _if_ the customer knows that they will never need the prescanning. This is the _ultimate_ in performance on this path. We anticipate some HPC customers who build their own kernels may want this option. > > Also is it better to > default to off or on for this code, what are the upsides and downsides > of that choice? > I'll update the commit message. But will also explain here. Enabling the code with the config option introduces a _slight_ performance impact but allows the user to determine if this congestion control fix should be active or not at run time. You are correct that most distros will enable the config options (default Yes). Therefore this becomes a system admin control item for the fabric being configured. After testing we found that this was the best compromise for most systems in terms of performance and flexibility. This is because prescanning is not required most of the time. Should a customer find that their topology and/or workload requires prescanning they can then enable this option at run time. This will result in a significant reduction of performance at the node level but may for those customers result in better overall fabric/cluster performance. So in conclusion we have 3 performance levels and the combination in this patch puts us at the "middle one". Thanks, Ira -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 05, 2015 at 12:02:25PM -0500, ira.weiny wrote: > On Thu, Nov 05, 2015 at 10:56:27AM +0300, Dan Carpenter wrote: > > On Wed, Nov 04, 2015 at 09:10:11PM -0500, ira.weiny@intel.com wrote: > > > From: Vennila Megavannan <vennila.megavannan@intel.com> > > > > > > Add a module paramter to toggle prescan/Fast ECN Detection. > > > > > > In addition change the PRESCAN_RXQ Kconfig default to "yes". > > > > > > Reviewed-by: Arthur Kepner<arthur.kepner@intel.com> > > > Reviewed-by: Mike Marciniszyn<mike.marciniszyn@intel.com> > > > Signed-off-by: Vennila Megavannan<vennila.megavannan@intel.com> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > Hm... In the original code we had the config entry but the code to > > support this was actually disabled. Now we are turning on the config > > entry by default but the module parameter defaults to disabled. I think > > the documentation is a bit tricky because it should say that actually > > enabling the config is not enough, it's still disabled. > > We tried to make it clear but obviously missed the mark. > > > > > Do we really need the config to be there? Distros are going to enable it. > > Who is it who wants to disable the config? > > The config is maintained to be able to fully remove the code to obtain the > maximum receive performance _if_ the customer knows that they will never need > the prescanning. This is the _ultimate_ in performance on this path. We > anticipate some HPC customers who build their own kernels may want this option. > > > > > Also is it better to > > default to off or on for this code, what are the upsides and downsides > > of that choice? > > > > I'll update the commit message. But will also explain here. > > Enabling the code with the config option introduces a _slight_ performance > impact but allows the user to determine if this congestion control fix should > be active or not at run time. You are correct that most distros will enable > the config options (default Yes). Therefore this becomes a system admin > control item for the fabric being configured. After testing we found that this > was the best compromise for most systems in terms of performance and > flexibility. This is because prescanning is not required most of the time. > Should a customer find that their topology and/or workload requires prescanning > they can then enable this option at run time. This will result in a > significant reduction of performance at the node level but may for those > customers result in better overall fabric/cluster performance. > > So in conclusion we have 3 performance levels and the combination in this patch > puts us at the "middle one". Can't you just make this a dynamic thing, not a build-time thing? No one will rebuild their kernels (distros forbid this for obvious support reasons), so making this work "automatically" is the best thing. Second best is having the ability to "tune" it by hand, but you really don't want that. Worse case is having a build-time option, which you chose :( thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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/staging/rdma/hfi1/Kconfig b/drivers/staging/rdma/hfi1/Kconfig index fd25078ee923..cfc9f9324b8d 100644 --- a/drivers/staging/rdma/hfi1/Kconfig +++ b/drivers/staging/rdma/hfi1/Kconfig @@ -26,12 +26,12 @@ config SDMA_VERBOSITY This is a configuration flag to enable verbose SDMA debug config PRESCAN_RXQ - bool "Enable prescanning of the RX queue for ECNs" + bool "Enable optional prescanning of the RX queue for ECNs" depends on INFINIBAND_HFI1 - default n + default y ---help--- - This option toggles the prescanning of the receive queue for - Explicit Congestion Notifications. If an ECN is detected, it - is processed as quickly as possible, the ECN is toggled off. - After the prescanning step, the receive queue is processed as - usual. + This option enables code for the prescanning of the receive queue for + Explicit Congestion Notifications. Pre-scanning can be controlled via a + module option at run time. If an ECN is detected, it is processed as + quickly as possible, the ECN is toggled off. After the prescanning + step, the receive queue is processed as usual. diff --git a/drivers/staging/rdma/hfi1/driver.c b/drivers/staging/rdma/hfi1/driver.c index 9a4ec09af020..4c1dd7130d7a 100644 --- a/drivers/staging/rdma/hfi1/driver.c +++ b/drivers/staging/rdma/hfi1/driver.c @@ -83,6 +83,14 @@ unsigned int hfi1_cu = 1; module_param_named(cu, hfi1_cu, uint, S_IRUGO); MODULE_PARM_DESC(cu, "Credit return units"); +#ifdef CONFIG_PRESCAN_RXQ +static unsigned int prescan_rx_queue; +module_param_named(prescan_rxq, prescan_rx_queue, uint, + S_IRUGO | S_IWUSR); +MODULE_PARM_DESC(prescan_rxq, + "Used to toggle rx prescan. Set to 1 to enable prescan"); +#endif /* CONFIG_PRESCAN_RXQ */ + unsigned long hfi1_cap_mask = HFI1_CAP_MASK_DEFAULT; static int hfi1_caps_set(const char *, const struct kernel_param *); static int hfi1_caps_get(char *, const struct kernel_param *); @@ -435,10 +443,8 @@ static inline void init_packet(struct hfi1_ctxtdata *rcd, } #ifndef CONFIG_PRESCAN_RXQ -static void prescan_rxq(struct hfi1_packet *packet) {} +#define prescan_rxq(packet) #else /* !CONFIG_PRESCAN_RXQ */ -static int prescan_receive_queue; - static void process_ecn(struct hfi1_qp *qp, struct hfi1_ib_header *hdr, struct hfi1_other_headers *ohdr, u64 rhf, u32 bth1, struct ib_grh *grh) @@ -541,15 +547,19 @@ static inline void update_ps_mdata(struct ps_mdata *mdata) * containing Excplicit Congestion Notifications (FECNs, or BECNs). * When an ECN is found, process the Congestion Notification, and toggle * it off. + * This is declared as a macro to allow quick checking of the module param and + * avoid the overhead of a function call if not enabled. */ -static void prescan_rxq(struct hfi1_packet *packet) +#define prescan_rxq(packet) \ + do { \ + if (prescan_rx_queue) \ + __prescan_rxq(packet); \ + } while (0) +static void __prescan_rxq(struct hfi1_packet *packet) { struct hfi1_ctxtdata *rcd = packet->rcd; struct ps_mdata mdata; - if (!prescan_receive_queue) - return; - init_ps_mdata(&mdata, packet); while (1) {