| Message ID | 1472774969-18997-7-git-send-email-knut.omang@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Sep 02, 2016 at 02:09:26AM +0200, Knut Omang wrote: > This commit removes the implementation and use of the modify_qp_mask > helper function from the generic OFED implementation and into individual > device drivers. > > Like with use of the ib_modify_qp_is_ok function it should be up to > each device driver how to handle bits set in the attribute masks. > > With the modify_qp_mask function applied in the generic code, > drivers would not see the bits that the user process actually sets. > > The restrictions imposed by the filter are also beyond what > is imposed by the Infiniband standard, and would also limit > future drivers or hardware from checking for unsupported or > invalid settings. I'm not excited about this direction. It is not OK for different drivers to use different mask algorithms, they must all behave the same. What is it you want to do differently, and why shouldn't the mlx drivers be updated to match that? Jason -- 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, 2016-09-01 at 20:13 -0600, Jason Gunthorpe wrote: > On Fri, Sep 02, 2016 at 02:09:26AM +0200, Knut Omang wrote: > > This commit removes the implementation and use of the modify_qp_mask > > helper function from the generic OFED implementation and into individual > > device drivers. > > > > Like with use of the ib_modify_qp_is_ok function it should be up to > > each device driver how to handle bits set in the attribute masks. > > > > With the modify_qp_mask function applied in the generic code, > > drivers would not see the bits that the user process actually sets. > > > > The restrictions imposed by the filter are also beyond what > > is imposed by the Infiniband standard, and would also limit > > future drivers or hardware from checking for unsupported or > > invalid settings. > > I'm not excited about this direction. It is not OK for different > drivers to use different mask algorithms, they must all behave the > same. The problem I am solving here is that SIF expects (and requires) some of the bits that the user correctly sets, but that Mellanox either ignore or need to be 0. So when that mask is applied to the user input, the value that reaches the SIF driver is not correct from SIFs perspective. If the values goes through as the ULP sets them, then all is fine. > What is it you want to do differently, and why shouldn't the mlx > drivers be updated to match that? I'd like the hardware specific part to decide which of the user bits that are interesting and which are not. Currently that information is lost on the way down to the hardware specific calls. Knut > > Jason -- 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 Fri, Sep 02, 2016 at 06:45:52AM +0200, Knut Omang wrote: > On Thu, 2016-09-01 at 20:13 -0600, Jason Gunthorpe wrote: > > On Fri, Sep 02, 2016 at 02:09:26AM +0200, Knut Omang wrote: > > > This commit removes the implementation and use of the modify_qp_mask > > > helper function from the generic OFED implementation and into individual > > > device drivers. > > > > > > Like with use of the ib_modify_qp_is_ok function it should be up to > > > each device driver how to handle bits set in the attribute masks. > > > > > > With the modify_qp_mask function applied in the generic code, > > > drivers would not see the bits that the user process actually sets. > > > > > > The restrictions imposed by the filter are also beyond what > > > is imposed by the Infiniband standard, and would also limit > > > future drivers or hardware from checking for unsupported or > > > invalid settings. > > > > I'm not excited about this direction. It is not OK for different > > drivers to use different mask algorithms, they must all behave the > > same. > > The problem I am solving here is that SIF expects (and requires) some of > the bits that the user correctly sets, but that Mellanox either ignore or need to > be 0. So when that mask is applied to the user input, the value that reaches > the SIF driver is not correct from SIFs perspective. > If the values goes through as the ULP sets them, then all is fine. I guess I still don't understand. SIF and mlx cannot have different behavior here. How is the application writer to know what to do? Can you give a concrete example of the problem? Jason -- 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 Fri, 2016-09-02 at 11:10 -0600, Jason Gunthorpe wrote: > On Fri, Sep 02, 2016 at 06:45:52AM +0200, Knut Omang wrote: > > On Thu, 2016-09-01 at 20:13 -0600, Jason Gunthorpe wrote: > > > On Fri, Sep 02, 2016 at 02:09:26AM +0200, Knut Omang wrote: > > > > This commit removes the implementation and use of the modify_qp_mask > > > > helper function from the generic OFED implementation and into individual > > > > device drivers. > > > > > > > > Like with use of the ib_modify_qp_is_ok function it should be up to > > > > each device driver how to handle bits set in the attribute masks. > > > > > > > > With the modify_qp_mask function applied in the generic code, > > > > drivers would not see the bits that the user process actually sets. > > > > > > > > The restrictions imposed by the filter are also beyond what > > > > is imposed by the Infiniband standard, and would also limit > > > > future drivers or hardware from checking for unsupported or > > > > invalid settings. > > > > > > I'm not excited about this direction. It is not OK for different > > > drivers to use different mask algorithms, they must all behave the > > > same. > > > > The problem I am solving here is that SIF expects (and requires) some of > > the bits that the user correctly sets, but that Mellanox either ignore or need to > > be 0. So when that mask is applied to the user input, the value that reaches > > the SIF driver is not correct from SIFs perspective. > > If the values goes through as the ULP sets them, then all is fine. > > I guess I still don't understand. SIF and mlx cannot have different > behavior here. Rest assure, being the new kid on the block here, of course we need to work with existing applications. We try to test with whatever we can get our hands on, and make sure we behave sensibly.. > How is the application writer to know what to do? This masking applies to XRC only. The application does not need to do anything, it treats the XRC QP as any other RC QP and attempt similar modify_qp operations. Mlx chooses to ignore some of the bits provided. > Can you give a concrete example of the problem? Maybe someone from Mellanox can explain the reason for having the filtering there in the first place, and only from user mode XRC QPs? Knut > Jason -- 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 Fri, 2016-09-02 at 19:35 +0200, Knut Omang wrote: > On Fri, 2016-09-02 at 11:10 -0600, Jason Gunthorpe wrote: > > On Fri, Sep 02, 2016 at 06:45:52AM +0200, Knut Omang wrote: > > > On Thu, 2016-09-01 at 20:13 -0600, Jason Gunthorpe wrote: > > > > On Fri, Sep 02, 2016 at 02:09:26AM +0200, Knut Omang wrote: > > > > > This commit removes the implementation and use of the modify_qp_mask > > > > > helper function from the generic OFED implementation and into individual > > > > > device drivers. > > > > > > > > > > Like with use of the ib_modify_qp_is_ok function it should be up to > > > > > each device driver how to handle bits set in the attribute masks. > > > > > > > > > > With the modify_qp_mask function applied in the generic code, > > > > > drivers would not see the bits that the user process actually sets. > > > > > > > > > > The restrictions imposed by the filter are also beyond what > > > > > is imposed by the Infiniband standard, and would also limit > > > > > future drivers or hardware from checking for unsupported or > > > > > invalid settings. > > > > > > > > I'm not excited about this direction. It is not OK for different > > > > drivers to use different mask algorithms, they must all behave the > > > > same. > > > > > > The problem I am solving here is that SIF expects (and requires) some of > > > the bits that the user correctly sets, but that Mellanox either ignore or need to > > > be 0. So when that mask is applied to the user input, the value that reaches > > > the SIF driver is not correct from SIFs perspective. > > > If the values goes through as the ULP sets them, then all is fine. > > > > I guess I still don't understand. SIF and mlx cannot have different > > behavior here. > > Rest assure, being the new kid on the block here, of course we need to > work with existing applications. We try to test with whatever we can get our > hands on, and make sure we behave sensibly.. > > > How is the application writer to know what to do? > > This masking applies to XRC only. > The application does not need to do anything, it treats the XRC QP as any other > RC QP and attempt similar modify_qp operations. > Mlx chooses to ignore some of the bits provided. > > > Can you give a concrete example of the problem? > > Maybe someone from Mellanox can explain the reason > for having the filtering there in the first place, > and only from user mode XRC QPs? Getting some help from git blame here: The modify_qp_mask function was introduced by this commit: 9977f4f64bfeb5d907a793a6880aab2d43b0bed2 by Sean Hefty. Can I ask why this function was introduced, and if it is still needed? Thanks, Knut > > Knut > > > Jason > -- > 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
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index dbc5885..35d18e0 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -2314,20 +2314,6 @@ out: return ret ? ret : in_len; } -/* Remove ignored fields set in the attribute mask */ -static int modify_qp_mask(enum ib_qp_type qp_type, int mask) -{ - switch (qp_type) { - case IB_QPT_XRC_INI: - return mask & ~(IB_QP_MAX_DEST_RD_ATOMIC | IB_QP_MIN_RNR_TIMER); - case IB_QPT_XRC_TGT: - return mask & ~(IB_QP_MAX_QP_RD_ATOMIC | IB_QP_RETRY_CNT | - IB_QP_RNR_RETRY); - default: - return mask; - } -} - ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file *file, struct ib_device *ib_dev, const char __user *buf, int in_len, @@ -2405,10 +2391,9 @@ ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file *file, ret = ib_resolve_eth_dmac(qp, attr, &cmd.attr_mask); if (ret) goto release_qp; - ret = qp->device->modify_qp(qp, attr, - modify_qp_mask(qp->qp_type, cmd.attr_mask), &udata); + ret = qp->device->modify_qp(qp, attr, cmd.attr_mask, &udata); } else { - ret = ib_modify_qp(qp, attr, modify_qp_mask(qp->qp_type, cmd.attr_mask)); + ret = ib_modify_qp(qp, attr, cmd.attr_mask); } if (ret) diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c index 363b1cb..3a5061f 100644 --- a/drivers/infiniband/hw/mlx4/qp.c +++ b/drivers/infiniband/hw/mlx4/qp.c @@ -2154,6 +2154,20 @@ out: return err; } +/* filter out ignored fields set in the attribute mask */ +static int modify_qp_mask(enum ib_qp_type qp_type, int mask) +{ + switch (qp_type) { + case IB_QPT_XRC_INI: + return mask & ~(IB_QP_MAX_DEST_RD_ATOMIC | IB_QP_MIN_RNR_TIMER); + case IB_QPT_XRC_TGT: + return mask & ~(IB_QP_MAX_QP_RD_ATOMIC | IB_QP_RETRY_CNT | + IB_QP_RNR_RETRY); + default: + return mask; + } +} + static int _mlx4_ib_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr, int attr_mask, struct ib_udata *udata) { @@ -2162,6 +2176,10 @@ static int _mlx4_ib_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr, enum ib_qp_state cur_state, new_state; int err = -EINVAL; int ll; + + if (udata) + attr_mask = modify_qp_mask(ibqp->qp_type, attr_mask); + mutex_lock(&qp->mutex); cur_state = attr_mask & IB_QP_CUR_STATE ? attr->cur_qp_state : qp->state; @@ -3501,4 +3519,3 @@ out: mutex_unlock(&qp->mutex); return err; } - diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c index 3a48d9db..5006ab0 100644 --- a/drivers/infiniband/hw/mlx5/qp.c +++ b/drivers/infiniband/hw/mlx5/qp.c @@ -2737,6 +2737,20 @@ out: return err; } +/* filter out ignored fields set in the attribute mask */ +static int modify_qp_mask(enum ib_qp_type qp_type, int mask) +{ + switch (qp_type) { + case IB_QPT_XRC_INI: + return mask & ~(IB_QP_MAX_DEST_RD_ATOMIC | IB_QP_MIN_RNR_TIMER); + case IB_QPT_XRC_TGT: + return mask & ~(IB_QP_MAX_QP_RD_ATOMIC | IB_QP_RETRY_CNT | + IB_QP_RNR_RETRY); + default: + return mask; + } +} + int mlx5_ib_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr, int attr_mask, struct ib_udata *udata) { @@ -2751,6 +2765,9 @@ int mlx5_ib_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr, if (ibqp->rwq_ind_tbl) return -ENOSYS; + if (udata) + attr_mask = modify_qp_mask(ibqp->qp_type, attr_mask); + if (unlikely(ibqp->qp_type == IB_QPT_GSI)) return mlx5_ib_gsi_modify_qp(ibqp, attr, attr_mask);
This commit removes the implementation and use of the modify_qp_mask helper function from the generic OFED implementation and into individual device drivers. Like with use of the ib_modify_qp_is_ok function it should be up to each device driver how to handle bits set in the attribute masks. With the modify_qp_mask function applied in the generic code, drivers would not see the bits that the user process actually sets. The restrictions imposed by the filter are also beyond what is imposed by the Infiniband standard, and would also limit future drivers or hardware from checking for unsupported or invalid settings. Signed-off-by: Knut Omang <knut.omang@oracle.com> --- drivers/infiniband/core/uverbs_cmd.c | 19 ++----------------- drivers/infiniband/hw/mlx4/qp.c | 19 ++++++++++++++++++- drivers/infiniband/hw/mlx5/qp.c | 17 +++++++++++++++++ 3 files changed, 37 insertions(+), 18 deletions(-)