| Message ID | 20190501095722.6902-8-michal.kalderon@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qed*: Improve performance on 100G link for offload protocols | expand |
From: Michal Kalderon <michal.kalderon@marvell.com> Date: Wed, 1 May 2019 12:57:19 +0300 > diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c > index d93c8a893a89..8bc6775abb79 100644 > --- a/drivers/infiniband/hw/qedr/main.c > +++ b/drivers/infiniband/hw/qedr/main.c > @@ -52,6 +52,10 @@ MODULE_DESCRIPTION("QLogic 40G/100G ROCE Driver"); > MODULE_AUTHOR("QLogic Corporation"); > MODULE_LICENSE("Dual BSD/GPL"); > > +static uint iwarp_cmt; > +module_param(iwarp_cmt, uint, 0444); > +MODULE_PARM_DESC(iwarp_cmt, " iWARP: Support CMT mode. 0 - Disabled, 1 - Enabled. Default: Disabled"); > + Sorry no, this is totally beneath us.
On Wed, May 01, 2019 at 08:35:22PM -0400, David Miller wrote: > From: Michal Kalderon <michal.kalderon@marvell.com> > Date: Wed, 1 May 2019 12:57:19 +0300 > > > diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c > > index d93c8a893a89..8bc6775abb79 100644 > > --- a/drivers/infiniband/hw/qedr/main.c > > +++ b/drivers/infiniband/hw/qedr/main.c > > @@ -52,6 +52,10 @@ MODULE_DESCRIPTION("QLogic 40G/100G ROCE Driver"); > > MODULE_AUTHOR("QLogic Corporation"); > > MODULE_LICENSE("Dual BSD/GPL"); > > > > +static uint iwarp_cmt; > > +module_param(iwarp_cmt, uint, 0444); > > +MODULE_PARM_DESC(iwarp_cmt, " iWARP: Support CMT mode. 0 - Disabled, 1 - Enabled. Default: Disabled"); > > + > > Sorry no, this is totally beneath us. It is not acceptable for RDMA too. Also please don't use comments inside function calls, it complicates various checkers without real need. dev->ops->iwarp_set_engine_affin(dev->cdev, true /* reset */); ^^^^^^^^^^^^^^ Thanks
> From: Leon Romanovsky <leon@kernel.org> > Sent: Thursday, May 2, 2019 8:13 AM > On Wed, May 01, 2019 at 08:35:22PM -0400, David Miller wrote: > > From: Michal Kalderon <michal.kalderon@marvell.com> > > Date: Wed, 1 May 2019 12:57:19 +0300 > > > > > diff --git a/drivers/infiniband/hw/qedr/main.c > > > b/drivers/infiniband/hw/qedr/main.c > > > index d93c8a893a89..8bc6775abb79 100644 > > > --- a/drivers/infiniband/hw/qedr/main.c > > > +++ b/drivers/infiniband/hw/qedr/main.c > > > @@ -52,6 +52,10 @@ MODULE_DESCRIPTION("QLogic 40G/100G ROCE > > > Driver"); MODULE_AUTHOR("QLogic Corporation"); > > > MODULE_LICENSE("Dual BSD/GPL"); > > > > > > +static uint iwarp_cmt; > > > +module_param(iwarp_cmt, uint, 0444); > MODULE_PARM_DESC(iwarp_cmt, " > > > +iWARP: Support CMT mode. 0 - Disabled, 1 - Enabled. Default: > > > +Disabled"); > > > + > > > > Sorry no, this is totally beneath us. > > It is not acceptable for RDMA too. Dave and Leon, This is a bit of a special case related specifically to our hardware. Enabling iWARP on this kind of configuration impacts L2 performance. We don't want this to happen implicitly once the rdma driver is loaded since that can happen automatically and could lead to unexpected behavior from user perspective. Therefore we need a way of giving the user control to decide whether they want iWARP at the cost of L2 performance degradation. We also need this information as soon as the iWARP device registers, so using the rdma-tool would be too late. If module parameter is not an option, could you please advise what would be ok ? ethtool private flags ? devlink ? thanks, Michal > > Also please don't use comments inside function calls, it complicates various > checkers without real need. > dev->ops->iwarp_set_engine_affin(dev->cdev, true /* reset */); > ^^^^^^^^^^^^^^ Thanks
On Thu, May 02, 2019 at 12:10:39PM +0000, Michal Kalderon wrote: > > From: Leon Romanovsky <leon@kernel.org> > > Sent: Thursday, May 2, 2019 8:13 AM > > On Wed, May 01, 2019 at 08:35:22PM -0400, David Miller wrote: > > > From: Michal Kalderon <michal.kalderon@marvell.com> > > > Date: Wed, 1 May 2019 12:57:19 +0300 > > > > > > > diff --git a/drivers/infiniband/hw/qedr/main.c > > > > b/drivers/infiniband/hw/qedr/main.c > > > > index d93c8a893a89..8bc6775abb79 100644 > > > > --- a/drivers/infiniband/hw/qedr/main.c > > > > +++ b/drivers/infiniband/hw/qedr/main.c > > > > @@ -52,6 +52,10 @@ MODULE_DESCRIPTION("QLogic 40G/100G ROCE > > > > Driver"); MODULE_AUTHOR("QLogic Corporation"); > > > > MODULE_LICENSE("Dual BSD/GPL"); > > > > > > > > +static uint iwarp_cmt; > > > > +module_param(iwarp_cmt, uint, 0444); > > MODULE_PARM_DESC(iwarp_cmt, " > > > > +iWARP: Support CMT mode. 0 - Disabled, 1 - Enabled. Default: > > > > +Disabled"); > > > > + > > > > > > Sorry no, this is totally beneath us. > > > > It is not acceptable for RDMA too. > > Dave and Leon, > > This is a bit of a special case related specifically to our hardware. > Enabling iWARP on this kind of configuration impacts L2 performance. > We don't want this to happen implicitly once the rdma driver is loaded since > that can happen automatically and could lead to unexpected behavior from user perspective. > Therefore we need a way of giving the user control to decide whether they want iWARP at the cost > of L2 performance degradation. > We also need this information as soon as the iWARP device registers, so using the rdma-tool would be too late. > > If module parameter is not an option, could you please advise what would be ok ? > ethtool private flags ? > devlink ? Yes, devlink params are modern way to have same functionality as module parameters. This patch can help you in order to get a sense of how to do it. https://lore.kernel.org/patchwork/patch/959195/ Thanks > > thanks, > Michal > > > > > Also please don't use comments inside function calls, it complicates various > > checkers without real need. > > dev->ops->iwarp_set_engine_affin(dev->cdev, true /* reset */); > > ^^^^^^^^^^^^^^ Thanks
> From: Leon Romanovsky <leon@kernel.org> > Sent: Thursday, May 2, 2019 3:31 PM > > On Thu, May 02, 2019 at 12:10:39PM +0000, Michal Kalderon wrote: > > > From: Leon Romanovsky <leon@kernel.org> > > > Sent: Thursday, May 2, 2019 8:13 AM > > > On Wed, May 01, 2019 at 08:35:22PM -0400, David Miller wrote: > > > > From: Michal Kalderon <michal.kalderon@marvell.com> > > > > Date: Wed, 1 May 2019 12:57:19 +0300 > > > > > > > > > diff --git a/drivers/infiniband/hw/qedr/main.c > > > > > b/drivers/infiniband/hw/qedr/main.c > > > > > index d93c8a893a89..8bc6775abb79 100644 > > > > > --- a/drivers/infiniband/hw/qedr/main.c > > > > > +++ b/drivers/infiniband/hw/qedr/main.c > > > > > @@ -52,6 +52,10 @@ MODULE_DESCRIPTION("QLogic 40G/100G > ROCE > > > > > Driver"); MODULE_AUTHOR("QLogic Corporation"); > > > > > MODULE_LICENSE("Dual BSD/GPL"); > > > > > > > > > > +static uint iwarp_cmt; > > > > > +module_param(iwarp_cmt, uint, 0444); > > > MODULE_PARM_DESC(iwarp_cmt, " > > > > > +iWARP: Support CMT mode. 0 - Disabled, 1 - Enabled. Default: > > > > > +Disabled"); > > > > > + > > > > > > > > Sorry no, this is totally beneath us. > > > > > > It is not acceptable for RDMA too. > > > > Dave and Leon, > > > > This is a bit of a special case related specifically to our hardware. > > Enabling iWARP on this kind of configuration impacts L2 performance. > > We don't want this to happen implicitly once the rdma driver is loaded > > since that can happen automatically and could lead to unexpected behavior > from user perspective. > > Therefore we need a way of giving the user control to decide whether > > they want iWARP at the cost of L2 performance degradation. > > We also need this information as soon as the iWARP device registers, so > using the rdma-tool would be too late. > > > > If module parameter is not an option, could you please advise what would > be ok ? > > ethtool private flags ? > > devlink ? > > Yes, devlink params are modern way to have same functionality as module > parameters. > > This patch can help you in order to get a sense of how to do it. > https://lore.kernel.org/patchwork/patch/959195/ > Thanks Leon, Michal > Thanks > > > > > thanks, > > Michal > > > > > > > > Also please don't use comments inside function calls, it complicates > > > various checkers without real need. > > > dev->ops->iwarp_set_engine_affin(dev->cdev, true /* reset */); > > > ^^^^^^^^^^^^^^ > > > Thanks
diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c index d93c8a893a89..8bc6775abb79 100644 --- a/drivers/infiniband/hw/qedr/main.c +++ b/drivers/infiniband/hw/qedr/main.c @@ -52,6 +52,10 @@ MODULE_DESCRIPTION("QLogic 40G/100G ROCE Driver"); MODULE_AUTHOR("QLogic Corporation"); MODULE_LICENSE("Dual BSD/GPL"); +static uint iwarp_cmt; +module_param(iwarp_cmt, uint, 0444); +MODULE_PARM_DESC(iwarp_cmt, " iWARP: Support CMT mode. 0 - Disabled, 1 - Enabled. Default: Disabled"); + #define QEDR_WQ_MULTIPLIER_DFT (3) static void qedr_ib_dispatch_event(struct qedr_dev *dev, u8 port_num, @@ -886,7 +890,21 @@ static struct qedr_dev *qedr_add(struct qed_dev *cdev, struct pci_dev *pdev, dev->user_dpm_enabled = dev_info.user_dpm_enabled; dev->rdma_type = dev_info.rdma_type; dev->num_hwfns = dev_info.common.num_hwfns; + + if (IS_IWARP(dev) && QEDR_IS_CMT(dev)) { + if (!iwarp_cmt) { + DP_ERR(dev, + "By default, iWARP is not supported over a 100G device. Can use the \"iwarp_cmt\" module parameter to enable it.\n"); + rc = -EINVAL; + goto init_err; + } + + rc = dev->ops->iwarp_set_engine_affin(cdev, false /* !reset */); + if (rc) + goto init_err; + } dev->affin_hwfn_idx = dev->ops->common->get_affin_hwfn_idx(cdev); + dev->rdma_ctx = dev->ops->rdma_get_rdma_ctx(cdev); dev->num_cnq = dev->ops->rdma_get_min_cnq_msix(cdev); @@ -947,6 +965,10 @@ static void qedr_remove(struct qedr_dev *dev) qedr_stop_hw(dev); qedr_sync_free_irqs(dev); qedr_free_resources(dev); + + if (IS_IWARP(dev) && QEDR_IS_CMT(dev) && iwarp_cmt) + dev->ops->iwarp_set_engine_affin(dev->cdev, true /* reset */); + ib_dealloc_device(&dev->ibdev); } diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c index e4d63359864e..1bcbb4ceeb2e 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c @@ -1916,6 +1916,32 @@ static int qed_roce_ll2_set_mac_filter(struct qed_dev *cdev, return rc; } +static int qed_iwarp_set_engine_affin(struct qed_dev *cdev, bool b_reset) +{ + enum qed_eng eng; + u8 ppfid = 0; + int rc; + + if (b_reset) + eng = QED_BOTH_ENG; + else + eng = cdev->l2_affin_hint ? QED_ENG1 : QED_ENG0; + + rc = qed_llh_set_ppfid_affinity(cdev, ppfid, eng); + if (rc) { + DP_NOTICE(cdev, + "Failed to set the engine affinity of ppfid %d\n", + ppfid); + return rc; + } + + DP_VERBOSE(cdev, (QED_MSG_RDMA | QED_MSG_SP), + "LLH: Set the engine affinity of non-RoCE packets as %d\n", + eng); + + return 0; +} + static const struct qed_rdma_ops qed_rdma_ops_pass = { .common = &qed_common_ops_pass, .fill_dev_info = &qed_fill_rdma_dev_info, @@ -1955,6 +1981,7 @@ static const struct qed_rdma_ops qed_rdma_ops_pass = { .ll2_set_fragment_of_tx_packet = &qed_ll2_set_fragment_of_tx_packet, .ll2_set_mac_filter = &qed_roce_ll2_set_mac_filter, .ll2_get_stats = &qed_ll2_get_stats, + .iwarp_set_engine_affin = &qed_iwarp_set_engine_affin, .iwarp_connect = &qed_iwarp_connect, .iwarp_create_listen = &qed_iwarp_create_listen, .iwarp_destroy_listen = &qed_iwarp_destroy_listen, diff --git a/include/linux/qed/qed_rdma_if.h b/include/linux/qed/qed_rdma_if.h index d15f8e4815e3..898f595ea3d6 100644 --- a/include/linux/qed/qed_rdma_if.h +++ b/include/linux/qed/qed_rdma_if.h @@ -670,6 +670,8 @@ struct qed_rdma_ops { int (*ll2_set_mac_filter)(struct qed_dev *cdev, u8 *old_mac_address, u8 *new_mac_address); + int (*iwarp_set_engine_affin)(struct qed_dev *cdev, bool b_reset); + int (*iwarp_connect)(void *rdma_cxt, struct qed_iwarp_connect_in *iparams, struct qed_iwarp_connect_out *oparams);