| Message ID | 1477909297-14491-3-git-send-email-leon@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Two (extremely) minor suggestions inline. Yuval On Mon, Oct 31, 2016 at 12:21:35PM +0200, Leon Romanovsky wrote: > From: Bodong Wang <bodong@mellanox.com> > > Add new member rate_limit to ib_qp_attr, it shows the packet pacing rate Suggesting to replace with: Add new member rate_limit to ib_qp_attr which holds the packet pacing rate > in Kbps, 0 means unlimited. > > IB_QP_RATE_LIMIT is added to ib_attr_mask, and it could be used by RAW Suggesting to replace with: IB_QP_RATE_LIMIT is added to ib_attr_mask and could be used by RAW > QPs when changing QP state from RTR to RTS, RTS to RTS. > > Signed-off-by: Bodong Wang <bodong@mellanox.com> > Reviewed-by: Matan Barak <matanb@mellanox.com> > Signed-off-by: Leon Romanovsky <leon@kernel.org> > --- > drivers/infiniband/core/verbs.c | 2 ++ > include/rdma/ib_verbs.h | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > index 8368764..3e688b3 100644 > --- a/drivers/infiniband/core/verbs.c > +++ b/drivers/infiniband/core/verbs.c > @@ -1014,6 +1014,7 @@ static const struct { > IB_QP_QKEY), > [IB_QPT_GSI] = (IB_QP_CUR_STATE | > IB_QP_QKEY), > + [IB_QPT_RAW_PACKET] = IB_QP_RATE_LIMIT, > } > } > }, > @@ -1047,6 +1048,7 @@ static const struct { > IB_QP_QKEY), > [IB_QPT_GSI] = (IB_QP_CUR_STATE | > IB_QP_QKEY), > + [IB_QPT_RAW_PACKET] = IB_QP_RATE_LIMIT, > } > }, > [IB_QPS_SQD] = { > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index 5ad43a4..a065361 100644 > --- a/include/rdma/ib_verbs.h > +++ b/include/rdma/ib_verbs.h > @@ -1102,6 +1102,7 @@ enum ib_qp_attr_mask { > IB_QP_RESERVED2 = (1<<22), > IB_QP_RESERVED3 = (1<<23), > IB_QP_RESERVED4 = (1<<24), > + IB_QP_RATE_LIMIT = (1<<25), > }; > > enum ib_qp_state { > @@ -1151,6 +1152,7 @@ struct ib_qp_attr { > u8 rnr_retry; > u8 alt_port_num; > u8 alt_timeout; > + u32 rate_limit; > }; > > enum ib_wr_opcode { > -- > 2.7.4 > > -- > 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 -- 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 Tue, Nov 01, 2016 at 12:06:08PM +0200, Yuval Shaia wrote: > Two (extremely) minor suggestions inline. > > Yuval > > On Mon, Oct 31, 2016 at 12:21:35PM +0200, Leon Romanovsky wrote: > > From: Bodong Wang <bodong@mellanox.com> > > > > Add new member rate_limit to ib_qp_attr, it shows the packet pacing rate > > Suggesting to replace with: > Add new member rate_limit to ib_qp_attr which holds the packet pacing rate > > > in Kbps, 0 means unlimited. > > > > IB_QP_RATE_LIMIT is added to ib_attr_mask, and it could be used by RAW > > Suggesting to replace with: > IB_QP_RATE_LIMIT is added to ib_attr_mask and could be used by RAW > > > QPs when changing QP state from RTR to RTS, RTS to RTS. > > > > Signed-off-by: Bodong Wang <bodong@mellanox.com> > > Reviewed-by: Matan Barak <matanb@mellanox.com> > > Signed-off-by: Leon Romanovsky <leon@kernel.org> Thanks Yuval, Doug, What do you expect from us? respin of this patch? Thanks
> enum ib_qp_state { > @@ -1151,6 +1152,7 @@ struct ib_qp_attr { > u8 rnr_retry; > u8 alt_port_num; > u8 alt_timeout; > + u32 rate_limit; > }; We already have ib_qp_attr::ib_ah_attr::static_rate, and that accounts for both the primary and alternate paths. We should not add a conflicting rate_limit field. Either use static_rate as defined by the spec, or replace/update it, with corresponding changes to how it is used in conjunction with SM data. -- 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 11/9/2016 11:27 AM, Hefty, Sean wrote: >> enum ib_qp_state { >> @@ -1151,6 +1152,7 @@ struct ib_qp_attr { >> u8 rnr_retry; >> u8 alt_port_num; >> u8 alt_timeout; >> + u32 rate_limit; >> }; > We already have ib_qp_attr::ib_ah_attr::static_rate, and that accounts for both the primary and alternate paths. We should not add a conflicting rate_limit field. Either use static_rate as defined by the spec, or replace/update it, with corresponding changes to how it is used in conjunction with SM data. They are different features. Static rate has a limitation on how many different speeds we could get, while packet pacing(rate limit) allows customer to set any number between the min and max range. -- 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/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c index 8368764..3e688b3 100644 --- a/drivers/infiniband/core/verbs.c +++ b/drivers/infiniband/core/verbs.c @@ -1014,6 +1014,7 @@ static const struct { IB_QP_QKEY), [IB_QPT_GSI] = (IB_QP_CUR_STATE | IB_QP_QKEY), + [IB_QPT_RAW_PACKET] = IB_QP_RATE_LIMIT, } } }, @@ -1047,6 +1048,7 @@ static const struct { IB_QP_QKEY), [IB_QPT_GSI] = (IB_QP_CUR_STATE | IB_QP_QKEY), + [IB_QPT_RAW_PACKET] = IB_QP_RATE_LIMIT, } }, [IB_QPS_SQD] = { diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 5ad43a4..a065361 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -1102,6 +1102,7 @@ enum ib_qp_attr_mask { IB_QP_RESERVED2 = (1<<22), IB_QP_RESERVED3 = (1<<23), IB_QP_RESERVED4 = (1<<24), + IB_QP_RATE_LIMIT = (1<<25), }; enum ib_qp_state { @@ -1151,6 +1152,7 @@ struct ib_qp_attr { u8 rnr_retry; u8 alt_port_num; u8 alt_timeout; + u32 rate_limit; }; enum ib_wr_opcode {