| Message ID | 1447086657-15358-2-git-send-email-matanb@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Nov 09, 2015 at 06:30:54PM +0200, Matan Barak wrote: > In order to create a CQ that supports timestamp, mlx5 needs to > support the extended create CQ command with the timestamp flag. > i> Signed-off-by: Matan Barak <matanb@mellanox.com> Acked-by: Eli Cohen <eli@mellanox.com> -- 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 Mon, Nov 09, 2015 at 06:30:54PM +0200, Matan Barak wrote: > In order to create a CQ that supports timestamp, mlx5 needs to > support the extended create CQ command with the timestamp flag. > > Signed-off-by: Matan Barak <matanb@mellanox.com> Acked-by: Eli Cohen <eli@mellanox.com> -- 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 Mon, Nov 09, 2015 at 06:30:54PM +0200, Matan Barak wrote: > In order to create a CQ that supports timestamp, mlx5 needs to > support the extended create CQ command with the timestamp flag. > > Signed-off-by: Matan Barak <matanb@mellanox.com> > drivers/infiniband/hw/mlx5/cq.c | 7 +++++++ > drivers/infiniband/hw/mlx5/main.c | 3 ++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c > index 2d0dbbf..674f857 100644 > +++ b/drivers/infiniband/hw/mlx5/cq.c > @@ -743,6 +743,10 @@ static void destroy_cq_kernel(struct mlx5_ib_dev *dev, struct mlx5_ib_cq *cq) > mlx5_db_free(dev->mdev, &cq->db); > } > > +enum { > + CQ_CREATE_FLAGS_SUPPORTED = IB_CQ_FLAGS_TIMESTAMP_COMPLETION > +}; > + > struct ib_cq *mlx5_ib_create_cq(struct ib_device *ibdev, > const struct ib_cq_init_attr *attr, > struct ib_ucontext *context, > @@ -766,6 +770,9 @@ struct ib_cq *mlx5_ib_create_cq(struct ib_device *ibdev, > if (entries < 0) > return ERR_PTR(-EINVAL); > > + if (attr->flags & ~CQ_CREATE_FLAGS_SUPPORTED) > + return ERR_PTR(-EINVAL); And this is what I was just mentioning to Eli, EINVAL is not the right return, and this same comment applies to the places where the above was copy and pasted into drivers during the ex patching. Try for EOPNOTSUPP maybe? 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 Mon, Nov 09, 2015 at 06:30:54PM +0200, Matan Barak wrote: > @@ -1385,7 +1385,8 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev) > (1ull << IB_USER_VERBS_CMD_CREATE_XSRQ) | > (1ull << IB_USER_VERBS_CMD_OPEN_QP); > dev->ib_dev.uverbs_ex_cmd_mask = > - (1ull << IB_USER_VERBS_EX_CMD_QUERY_DEVICE); > + (1ull << IB_USER_VERBS_EX_CMD_QUERY_DEVICE) | > + (1ull << IB_USER_VERBS_EX_CMD_CREATE_CQ); Eli posted a series that gets rid of this stuff, can you please coordinate? 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 Tue, Nov 10, 2015 at 1:22 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Mon, Nov 09, 2015 at 06:30:54PM +0200, Matan Barak wrote: >> In order to create a CQ that supports timestamp, mlx5 needs to >> support the extended create CQ command with the timestamp flag. >> >> Signed-off-by: Matan Barak <matanb@mellanox.com> >> drivers/infiniband/hw/mlx5/cq.c | 7 +++++++ >> drivers/infiniband/hw/mlx5/main.c | 3 ++- >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c >> index 2d0dbbf..674f857 100644 >> +++ b/drivers/infiniband/hw/mlx5/cq.c >> @@ -743,6 +743,10 @@ static void destroy_cq_kernel(struct mlx5_ib_dev *dev, struct mlx5_ib_cq *cq) >> mlx5_db_free(dev->mdev, &cq->db); >> } >> >> +enum { >> + CQ_CREATE_FLAGS_SUPPORTED = IB_CQ_FLAGS_TIMESTAMP_COMPLETION >> +}; >> + >> struct ib_cq *mlx5_ib_create_cq(struct ib_device *ibdev, >> const struct ib_cq_init_attr *attr, >> struct ib_ucontext *context, >> @@ -766,6 +770,9 @@ struct ib_cq *mlx5_ib_create_cq(struct ib_device *ibdev, >> if (entries < 0) >> return ERR_PTR(-EINVAL); >> >> + if (attr->flags & ~CQ_CREATE_FLAGS_SUPPORTED) >> + return ERR_PTR(-EINVAL); > > And this is what I was just mentioning to Eli, EINVAL is not the right > return, and this same comment applies to the places where the above > was copy and pasted into drivers during the ex patching. > > Try for EOPNOTSUPP maybe? > Agree. Thanks, I'll fix. > Jason Matan > -- > 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 10, 2015 at 1:24 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Mon, Nov 09, 2015 at 06:30:54PM +0200, Matan Barak wrote: >> @@ -1385,7 +1385,8 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev) >> (1ull << IB_USER_VERBS_CMD_CREATE_XSRQ) | >> (1ull << IB_USER_VERBS_CMD_OPEN_QP); >> dev->ib_dev.uverbs_ex_cmd_mask = >> - (1ull << IB_USER_VERBS_EX_CMD_QUERY_DEVICE); >> + (1ull << IB_USER_VERBS_EX_CMD_QUERY_DEVICE) | >> + (1ull << IB_USER_VERBS_EX_CMD_CREATE_CQ); > > Eli posted a series that gets rid of this stuff, can you please > coordinate? > It'll create a dependency between this series and Eli's series, but I'll change that. > Jason Matan > -- > 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
> +enum { > + CQ_CREATE_FLAGS_SUPPORTED = IB_CQ_FLAGS_TIMESTAMP_COMPLETION > +}; How does userspace know the value of IB_CQ_FLAGS_TIMESTAMP_COMPLETION? It's not defined in any UAPI header. -- 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 19, 2015 at 4:54 PM, Christoph Hellwig <hch@infradead.org> wrote: >> +enum { >> + CQ_CREATE_FLAGS_SUPPORTED = IB_CQ_FLAGS_TIMESTAMP_COMPLETION >> +}; > > > How does userspace know the value of IB_CQ_FLAGS_TIMESTAMP_COMPLETION? > > It's not defined in any UAPI header. Currently, a lot of flags are defined both in rdma/ib_verbs.h and are redefined as part of libibverbs. For example, IB_ODP_SUPPORT is defined in rdma/ib_verbs.h and redefined in libibverbs. We took the same approach here. We could put all of these flags in uverbs, but I think we should be consistent. So either only uapi exclusive structures should be placed in uapi or all uapi related data should be placed there. Regards, Matan > -- > 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
Hi Matan, IB_CQ_FLAGS_TIMESTAMP_COMPLETION is _only_ used in the uverbs interface. But other than that userspace ABIs really must go to the UAPI headers, otherwise they will be broken sooner or later. RDMA is an unfortunate oddball under the kernel subsystems that's been ignoring the rule for now. Please ensure at least new interfaces follow this principle, and old code will need to be fixed up as well. -- 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 Sun, Nov 22, 2015 at 12:15 PM, Christoph Hellwig <hch@infradead.org> wrote: > Hi Matan, > > IB_CQ_FLAGS_TIMESTAMP_COMPLETION is _only_ used in the uverbs interface. > Currently this is used only for uverbs interface. The reason for that is that we've only pushed completion timestamping for user-space support. However, there's no reason not to add support for completion timestamping for ULPs. Therefore, the same flag could be relevant for ULPs as well. > But other than that userspace ABIs really must go to the UAPI headers, > otherwise they will be broken sooner or later. RDMA is an unfortunate > oddball under the kernel subsystems that's been ignoring the rule for So do you imply ib_verbs.h should include ib_user_verbs.h? > now. Please ensure at least new interfaces follow this principle, and > old code will need to be fixed up as well. Ok, I'll follow this principle for new features. Matan -- 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/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c index 2d0dbbf..674f857 100644 --- a/drivers/infiniband/hw/mlx5/cq.c +++ b/drivers/infiniband/hw/mlx5/cq.c @@ -743,6 +743,10 @@ static void destroy_cq_kernel(struct mlx5_ib_dev *dev, struct mlx5_ib_cq *cq) mlx5_db_free(dev->mdev, &cq->db); } +enum { + CQ_CREATE_FLAGS_SUPPORTED = IB_CQ_FLAGS_TIMESTAMP_COMPLETION +}; + struct ib_cq *mlx5_ib_create_cq(struct ib_device *ibdev, const struct ib_cq_init_attr *attr, struct ib_ucontext *context, @@ -766,6 +770,9 @@ struct ib_cq *mlx5_ib_create_cq(struct ib_device *ibdev, if (entries < 0) return ERR_PTR(-EINVAL); + if (attr->flags & ~CQ_CREATE_FLAGS_SUPPORTED) + return ERR_PTR(-EINVAL); + entries = roundup_pow_of_two(entries + 1); if (entries > (1 << MLX5_CAP_GEN(dev->mdev, log_max_cq_sz))) return ERR_PTR(-EINVAL); diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c index f1ccd40..05f00da 100644 --- a/drivers/infiniband/hw/mlx5/main.c +++ b/drivers/infiniband/hw/mlx5/main.c @@ -1385,7 +1385,8 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev) (1ull << IB_USER_VERBS_CMD_CREATE_XSRQ) | (1ull << IB_USER_VERBS_CMD_OPEN_QP); dev->ib_dev.uverbs_ex_cmd_mask = - (1ull << IB_USER_VERBS_EX_CMD_QUERY_DEVICE); + (1ull << IB_USER_VERBS_EX_CMD_QUERY_DEVICE) | + (1ull << IB_USER_VERBS_EX_CMD_CREATE_CQ); dev->ib_dev.query_device = mlx5_ib_query_device; dev->ib_dev.query_port = mlx5_ib_query_port;
In order to create a CQ that supports timestamp, mlx5 needs to support the extended create CQ command with the timestamp flag. Signed-off-by: Matan Barak <matanb@mellanox.com> --- drivers/infiniband/hw/mlx5/cq.c | 7 +++++++ drivers/infiniband/hw/mlx5/main.c | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-)