| Message ID | 20211221173913.1386261-1-yanjun.zhu@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Leon Romanovsky |
| Headers | show |
| Series | [PATCHv3,1/1] RDMA/irdma: Make the source udp port vary | expand |
> Subject: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary > > From: Zhu Yanjun <yanjun.zhu@linux.dev> > > Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html, > get the source udp port number for a QP based on the grh.flow_label or > lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues. > The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp > source port when set path") is a standard way. So it is also adopted in this > commit. > > Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev> > --- > V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in > V2->ah_flags > V1->V2: Adopt a standard method to get udp source port. > --- > drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > Reviewed-by: Mustafa Ismail <mustafa.ismail@intel.com>
On Tue, Dec 21, 2021 at 12:39:13PM -0500, yanjun.zhu@linux.dev wrote: > From: Zhu Yanjun <yanjun.zhu@linux.dev> > > Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html, Please use lore.kernel.org links. They have all chances to outlive spinics. > get the source udp port number for a QP based on the grh.flow_label or > lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues. > The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp > source port when set path") is a standard way. So it is also adopted in > this commit. > > Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev> > --- > V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in ah_flags > V1->V2: Adopt a standard method to get udp source port. > --- > drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c > index 8cd5f9261692..31039b295206 100644 > --- a/drivers/infiniband/hw/irdma/verbs.c > +++ b/drivers/infiniband/hw/irdma/verbs.c > @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev, u32 port, u16 index, > return 0; > } > > + > +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn) > +{ > + if (!fl) > + fl = rdma_calc_flow_label(lqpn, rqpn); > + > + return rdma_flow_label_to_udp_sport(fl); > +} > + > /** > * irdma_modify_qp_roce - modify qp request > * @ibqp: qp's pointer for modify > @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct ib_qp_attr *attr, > > memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah)); > if (attr->ah_attr.ah_flags & IB_AH_GRH) { > + u32 fl = udp_info->flow_label; > + u32 lqp = ibqp->qp_num; > + u32 rqp = roce_info->dest_qp; > + I don't see too much value in these extra variables and extra function that is the same as get_udp_sport() from hns. It is worth to add new function to ib_verbs.h and reuse in both drivers. Thanks > + udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp); > udp_info->ttl = attr->ah_attr.grh.hop_limit; > udp_info->flow_label = attr->ah_attr.grh.flow_label; > udp_info->tos = attr->ah_attr.grh.traffic_class; > -- > 2.27.0 >
January 3, 2022 5:47 PM, "Leon Romanovsky" <leon@kernel.org> wrote: > On Tue, Dec 21, 2021 at 12:39:13PM -0500, yanjun.zhu@linux.dev wrote: > >> From: Zhu Yanjun <yanjun.zhu@linux.dev> >> >> Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html, > > Please use lore.kernel.org links. They have all chances to outlive spinics. > >> get the source udp port number for a QP based on the grh.flow_label or >> lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues. >> The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp >> source port when set path") is a standard way. So it is also adopted in >> this commit. >> >> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev> >> --- >> V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in ah_flags >> V1->V2: Adopt a standard method to get udp source port. >> --- >> drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c >> index 8cd5f9261692..31039b295206 100644 >> --- a/drivers/infiniband/hw/irdma/verbs.c >> +++ b/drivers/infiniband/hw/irdma/verbs.c >> @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev, u32 port, u16 index, >> return 0; >> } >> >> + >> +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn) >> +{ >> + if (!fl) >> + fl = rdma_calc_flow_label(lqpn, rqpn); >> + >> + return rdma_flow_label_to_udp_sport(fl); >> +} >> + >> /** >> * irdma_modify_qp_roce - modify qp request >> * @ibqp: qp's pointer for modify >> @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct ib_qp_attr *attr, >> >> memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah)); >> if (attr->ah_attr.ah_flags & IB_AH_GRH) { >> + u32 fl = udp_info->flow_label; >> + u32 lqp = ibqp->qp_num; >> + u32 rqp = roce_info->dest_qp; >> + > > I don't see too much value in these extra variables and extra function > that is the same as get_udp_sport() from hns. > > It is worth to add new function to ib_verbs.h and reuse in both drivers. Do you mean the following function should be added into ib_verbs.h? " static inline u16 rdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn) { if (!fl) fl = rdma_calc_flow_label(lqpn, rqpn); return rdma_flow_label_to_udp_sport(fl); } " Then in hns, rxe and irdma, this function is called to get udp source port? If so, I will send new patches. Thanks. Zhu Yanjun > > Thanks > >> + udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp); >> udp_info->ttl = attr->ah_attr.grh.hop_limit; >> udp_info->flow_label = attr->ah_attr.grh.flow_label; >> udp_info->tos = attr->ah_attr.grh.traffic_class; >> -- >> 2.27.0
On Tue, Jan 04, 2022 at 07:51:54AM +0000, yanjun.zhu@linux.dev wrote: > January 3, 2022 5:47 PM, "Leon Romanovsky" <leon@kernel.org> wrote: > > > On Tue, Dec 21, 2021 at 12:39:13PM -0500, yanjun.zhu@linux.dev wrote: > > > >> From: Zhu Yanjun <yanjun.zhu@linux.dev> > >> > >> Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html, > > > > Please use lore.kernel.org links. They have all chances to outlive spinics. > > > >> get the source udp port number for a QP based on the grh.flow_label or > >> lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues. > >> The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp > >> source port when set path") is a standard way. So it is also adopted in > >> this commit. > >> > >> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev> > >> --- > >> V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in ah_flags > >> V1->V2: Adopt a standard method to get udp source port. > >> --- > >> drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c > >> index 8cd5f9261692..31039b295206 100644 > >> --- a/drivers/infiniband/hw/irdma/verbs.c > >> +++ b/drivers/infiniband/hw/irdma/verbs.c > >> @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev, u32 port, u16 index, > >> return 0; > >> } > >> > >> + > >> +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn) > >> +{ > >> + if (!fl) > >> + fl = rdma_calc_flow_label(lqpn, rqpn); > >> + > >> + return rdma_flow_label_to_udp_sport(fl); > >> +} > >> + > >> /** > >> * irdma_modify_qp_roce - modify qp request > >> * @ibqp: qp's pointer for modify > >> @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct ib_qp_attr *attr, > >> > >> memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah)); > >> if (attr->ah_attr.ah_flags & IB_AH_GRH) { > >> + u32 fl = udp_info->flow_label; > >> + u32 lqp = ibqp->qp_num; > >> + u32 rqp = roce_info->dest_qp; > >> + > > > > I don't see too much value in these extra variables and extra function > > that is the same as get_udp_sport() from hns. > > > > It is worth to add new function to ib_verbs.h and reuse in both drivers. > > Do you mean the following function should be added into ib_verbs.h? > > " > static inline u16 rdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn) > { > if (!fl) > fl = rdma_calc_flow_label(lqpn, rqpn); > > return rdma_flow_label_to_udp_sport(fl); > } > " > Then in hns, rxe and irdma, this function is called to get udp source port? > If so, I will send new patches. Yes and group all these patches in one series, please. Thanks > > Thanks. > Zhu Yanjun > > > > > Thanks > > > >> + udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp); > >> udp_info->ttl = attr->ah_attr.grh.hop_limit; > >> udp_info->flow_label = attr->ah_attr.grh.flow_label; > >> udp_info->tos = attr->ah_attr.grh.traffic_class; > >> -- > >> 2.27.0
> Subject: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary > > From: Zhu Yanjun <yanjun.zhu@linux.dev> > > Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html, > get the source udp port number for a QP based on the grh.flow_label or > lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues. > The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp > source port when set path") is a standard way. So it is also adopted in this > commit. > > Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev> > --- > V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in > V2->ah_flags > V1->V2: Adopt a standard method to get udp source port. > --- > drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c > index 8cd5f9261692..31039b295206 100644 > --- a/drivers/infiniband/hw/irdma/verbs.c > +++ b/drivers/infiniband/hw/irdma/verbs.c > @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev, > u32 port, u16 index, > return 0; > } > > + > +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn) { > + if (!fl) > + fl = rdma_calc_flow_label(lqpn, rqpn); > + > + return rdma_flow_label_to_udp_sport(fl); } > + > /** > * irdma_modify_qp_roce - modify qp request > * @ibqp: qp's pointer for modify > @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct > ib_qp_attr *attr, > > memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah)); > if (attr->ah_attr.ah_flags & IB_AH_GRH) { > + u32 fl = udp_info->flow_label; > + u32 lqp = ibqp->qp_num; > + u32 rqp = roce_info->dest_qp; > + > + udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp); The flow label used in function is not right as udp_info->flow_label is only set a few lines below. Kindly fold this fix into your next revision. > udp_info->ttl = attr->ah_attr.grh.hop_limit; > udp_info->flow_label = attr->ah_attr.grh.flow_label; > udp_info->tos = attr->ah_attr.grh.traffic_class; > -- > 2.27.0
在 2022/1/4 23:07, Saleem, Shiraz 写道: >> Subject: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary >> >> From: Zhu Yanjun <yanjun.zhu@linux.dev> >> >> Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html, >> get the source udp port number for a QP based on the grh.flow_label or >> lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues. >> The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp >> source port when set path") is a standard way. So it is also adopted in this >> commit. >> >> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev> >> --- >> V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in >> V2->ah_flags >> V1->V2: Adopt a standard method to get udp source port. >> --- >> drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c >> index 8cd5f9261692..31039b295206 100644 >> --- a/drivers/infiniband/hw/irdma/verbs.c >> +++ b/drivers/infiniband/hw/irdma/verbs.c >> @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev, >> u32 port, u16 index, >> return 0; >> } >> >> + >> +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn) { >> + if (!fl) >> + fl = rdma_calc_flow_label(lqpn, rqpn); >> + >> + return rdma_flow_label_to_udp_sport(fl); } >> + >> /** >> * irdma_modify_qp_roce - modify qp request >> * @ibqp: qp's pointer for modify >> @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct >> ib_qp_attr *attr, >> >> memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah)); >> if (attr->ah_attr.ah_flags & IB_AH_GRH) { >> + u32 fl = udp_info->flow_label; >> + u32 lqp = ibqp->qp_num; >> + u32 rqp = roce_info->dest_qp; >> + >> + udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp); > The flow label used in function is not right as udp_info->flow_label is only set a few lines below. Kindly fold this fix into your next revision. Thanks. " attr->ah_attr.grh.flow_label " should be OK. Zhu Yanjun > >> udp_info->ttl = attr->ah_attr.grh.hop_limit; >> udp_info->flow_label = attr->ah_attr.grh.flow_label; >> udp_info->tos = attr->ah_attr.grh.traffic_class; >> -- >> 2.27.0
diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c index 8cd5f9261692..31039b295206 100644 --- a/drivers/infiniband/hw/irdma/verbs.c +++ b/drivers/infiniband/hw/irdma/verbs.c @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev, u32 port, u16 index, return 0; } + +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn) +{ + if (!fl) + fl = rdma_calc_flow_label(lqpn, rqpn); + + return rdma_flow_label_to_udp_sport(fl); +} + /** * irdma_modify_qp_roce - modify qp request * @ibqp: qp's pointer for modify @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct ib_qp_attr *attr, memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah)); if (attr->ah_attr.ah_flags & IB_AH_GRH) { + u32 fl = udp_info->flow_label; + u32 lqp = ibqp->qp_num; + u32 rqp = roce_info->dest_qp; + + udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp); udp_info->ttl = attr->ah_attr.grh.hop_limit; udp_info->flow_label = attr->ah_attr.grh.flow_label; udp_info->tos = attr->ah_attr.grh.traffic_class;