| Message ID | 20181204221443.11392.17827.stgit@scvm10.sc.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | hfi1verbs: Update rvt cq headers in rdma_core hfi1 provider | expand |
On Tue, Dec 04, 2018 at 02:14:43PM -0800, Kamenee Arumugam wrote: > hfi1_wc struct is the same struct as ib_uverbs_wc and hfi1_cq_wc > is not in the uapi header. Remove hfi1_wc and hfi1_cq_wc struct > from providers. Include rvt-abi.h header that have rvt_cq_wc > struct definition into kernel-headers. > > The member ib_uverbs_wc arrray in rvt_cq_wc is replaced with > one in rvt-abi. Therefore,plus one to the number of cq entries. > > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com> > Signed-off-by: Kamenee Arumugam <kamenee.arumugam@intel.com> > --- > kernel-headers/CMakeLists.txt | 1 + > kernel-headers/rdma/rvt-abi.h | 31 +++++++++++++++++++++++++++++++ > providers/hfi1verbs/hfiverbs.h | 37 +++++-------------------------------- > providers/hfi1verbs/verbs.c | 23 +++++++++++++---------- > 4 files changed, 50 insertions(+), 42 deletions(-) > create mode 100755 kernel-headers/rdma/rvt-abi.h Since these need a kernel patch can you remark which kernel patch this is matched with? Also changes to kernel-headers should be split into their own patch created by the kernel-headers/update script as the first patch in your series. Jason
> Subject: Re: [PATCH] hfi1verbs: Update rvt cq headers in rdma_core hfi1 provider > Since these need a kernel patch can you remark which kernel patch this is matched with? Here is the kernel patch: https://marc.info/?l=linux-rdma&m=154402265822649&w=2 > Also changes to kernel-headers should be split into their own patch created by the kernel-headers/update script as the first patch in your series. It there a README or documentation on the process for keeping kernel-headers in sync? So is this the sequence?: - Patch the kernel * Does the patch need to land in an external tree? Which tree? - Use the update script to sync the kernel-headers Since you want one file, it would seem we want both of the kernel patches first?
On Wed, Dec 05, 2018 at 03:30:38PM +0000, Arumugam, Kamenee wrote: > > Subject: Re: [PATCH] hfi1verbs: Update rvt cq headers in rdma_core hfi1 provider > > Since these need a kernel patch can you remark which kernel patch this is matched with? > > Here is the kernel patch: https://marc.info/?l=linux-rdma&m=154402265822649&w=2 > > > Also changes to kernel-headers should be split into their own patch created by the kernel-headers/update script as the first patch in your series. > > It there a README or documentation on the process for keeping kernel-headers in sync? Ah I wish there was, want to write one? > So is this the sequence?: > - Patch the kernel > * Does the patch need to land in an external tree? Which tree? > - Use the update script to sync the kernel-headers Not quite. Patch *your* kernel with the required kernel patches for the series Run $ kernel-headers/update --not-final ../kernel HEAD This creates the 'Updated kernel headers' commit you need Apply the patches in your series. Push to github and tag your PR with 'needs-kernel-patch'. Post to the mailing list with 'git send-email' When the kernel patch is accepted the you or the maintainer will do 'git rebase' and exec instead of pick the first commit with: exec kernel-headers/update ../kernel 12345566 To build the final commit and remove the 'needs-kernel-patch' tag. That gets force pushed to the PR and the PR merged. This keeps the headers in sync between the two projects. Jason
On Thu, 2018-12-06 at 20:40 -0700, Jason Gunthorpe wrote: > On Wed, Dec 05, 2018 at 03:30:38PM +0000, Arumugam, Kamenee wrote: > > > Subject: Re: [PATCH] hfi1verbs: Update rvt cq headers in rdma_core hfi1 provider > > > Since these need a kernel patch can you remark which kernel patch this is matched with? > > > > Here is the kernel patch: https://marc.info/?l=linux-rdma&m=154402265822649&w=2 > > > > > Also changes to kernel-headers should be split into their own patch created by the kernel-headers/update script as the first patch in your series. > > > > It there a README or documentation on the process for keeping kernel-headers in sync? > > Ah I wish there was, want to write one? > > > So is this the sequence?: > > - Patch the kernel > > * Does the patch need to land in an external tree? Which tree? > > - Use the update script to sync the kernel-headers > > Not quite. > > Patch *your* kernel with the required kernel patches for the series > > Run > > $ kernel-headers/update --not-final ../kernel HEAD > > This creates the 'Updated kernel headers' commit you need > > Apply the patches in your series. > > Push to github and tag your PR with 'needs-kernel-patch'. Post to the > mailing list with 'git send-email' > > When the kernel patch is accepted the you or the maintainer will do > 'git rebase' and exec instead of pick the first commit with: > > exec kernel-headers/update ../kernel 12345566 > > To build the final commit and remove the 'needs-kernel-patch' tag. > > That gets force pushed to the PR and the PR merged. > > This keeps the headers in sync between the two projects. > > Jason That seems entirely more complex than it should be. When you or I setup that level of complexity, it's not a big deal because we do this all the time. When you push that level of complexity onto the submitters, some of whom are going to be occasionally submitting uapi updates at best, then it's a recipe for botched updates. What if we create a git hook that tracks any updates to our local for- next branch and whenever we do an update that modifies anything in include/uapi/rdma it triggers a message to perform a headers update in rdma-core? Then we just jump over to that repo, git pull to make sure we're up to date with merges that have been done via the web interface, run kernel-headers/update ../kernel HEAD, git push, done. The only downside here is that the submitter will still need to have a header update patch in their pull request just so it will build/test prior to the official acceptance of the upstream patch. If I could do away with that too, I would. I'm just concerned that getting everyone out there to know how to use yet another tool and yet another exact sequence of operations is going to be a loosing battle.
On Wed, Dec 12, 2018 at 12:42:30PM -0500, Doug Ledford wrote: > What if we create a git hook that tracks any updates to our local for- > next branch and whenever we do an update that modifies anything in > include/uapi/rdma it triggers a message to perform a headers update in > rdma-core? This has to be done by the submitter before kernel patches are accepted because we require PR's for userspace compoments of kernel features before applying the kernel features.. So, the developer has to put the right kernel headers into their rdma-core to develop, test and push their rdma-core work.. The update script is the simplest way to do that I can see.. I guess we could let people hand roll their kernel update patches (but why?) so long as the maintainer re-writes them. Just getting people to put their kernel header updates into a single patch is an education process that isn't so easy. Jason
On 12/12/2018 6:19 PM, Jason Gunthorpe wrote: > On Wed, Dec 12, 2018 at 12:42:30PM -0500, Doug Ledford wrote: > >> What if we create a git hook that tracks any updates to our local for- >> next branch and whenever we do an update that modifies anything in >> include/uapi/rdma it triggers a message to perform a headers update in >> rdma-core? > > This has to be done by the submitter before kernel patches are > accepted because we require PR's for userspace compoments of kernel > features before applying the kernel features.. > > So, the developer has to put the right kernel headers into their > rdma-core to develop, test and push their rdma-core work.. > > The update script is the simplest way to do that I can see.. > > I guess we could let people hand roll their kernel update patches (but > why?) so long as the maintainer re-writes them. > > Just getting people to put their kernel header updates into a single > patch is an education process that isn't so easy. As long as there is a README that explains step by step what you guys want that should suffice. Yes it's a complicated process, but one that isn't done all that often so it's manageable. -Denny
diff --git a/kernel-headers/CMakeLists.txt b/kernel-headers/CMakeLists.txt index 16ba165..631db9c 100644 --- a/kernel-headers/CMakeLists.txt +++ b/kernel-headers/CMakeLists.txt @@ -24,6 +24,7 @@ publish_internal_headers(rdma rdma/rdma_user_ioctl_cmds.h rdma/rdma_user_rxe.h rdma/vmw_pvrdma-abi.h + rdma/rvt-abi.h ) publish_internal_headers(rdma/hfi diff --git a/kernel-headers/rdma/rvt-abi.h b/kernel-headers/rdma/rvt-abi.h new file mode 100755 index 0000000..c30f2fe --- /dev/null +++ b/kernel-headers/rdma/rvt-abi.h @@ -0,0 +1,31 @@ +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */ + +/* + * This file contains defines, structures, etc. that are used + * to communicate between kernel and user code. + */ + +#ifndef RVT_ABI_USER_H +#define RVT_ABI_USER_H + +#include <linux/types.h> +#include <rdma/ib_user_verbs.h> +#ifndef RDMA_ATOMIC_UAPI +#define RDMA_ATOMIC_UAPI(_type, _name) _type _name +#endif +/* + * This structure is used to contain the head pointer, tail pointer, + * and completion queue entries as a single memory allocation so + * it can be mmap'ed into user space. + */ +struct rvt_cq_wc { + /* index of next entry to fill */ + RDMA_ATOMIC_UAPI(__u32, head); + /* index of next ib_poll_cq() entry */ + RDMA_ATOMIC_UAPI(__u32, tail); + + /* these are actually size ibcq.cqe + 1 */ + struct ib_uverbs_wc uqueue[0]; +}; + +#endif /* RVT_ABI_USER_H */ diff --git a/providers/hfi1verbs/hfiverbs.h b/providers/hfi1verbs/hfiverbs.h index 070a01c..7ce27c8 100644 --- a/providers/hfi1verbs/hfiverbs.h +++ b/providers/hfi1verbs/hfiverbs.h @@ -65,7 +65,8 @@ #include <infiniband/driver.h> #include <infiniband/verbs.h> - +#define RDMA_ATOMIC_UAPI(_type, _name) _Atomic(_type) _name +#include "rdma/rvt-abi.h" #define PFX "hfi1: " struct hfi1_device { @@ -77,39 +78,11 @@ struct hfi1_context { struct verbs_context ibv_ctx; }; -/* - * This structure needs to have the same size and offsets as - * the kernel's ib_wc structure since it is memory mapped. - */ -struct hfi1_wc { - uint64_t wr_id; - enum ibv_wc_status status; - enum ibv_wc_opcode opcode; - uint32_t vendor_err; - uint32_t byte_len; - uint32_t imm_data; /* in network byte order */ - uint32_t qp_num; - uint32_t src_qp; - enum ibv_wc_flags wc_flags; - uint16_t pkey_index; - uint16_t slid; - uint8_t sl; - uint8_t dlid_path_bits; - uint8_t port_num; -}; - -struct hfi1_cq_wc { - _Atomic(uint32_t) head; - _Atomic(uint32_t) tail; - struct hfi1_wc queue[1]; -}; - struct hfi1_cq { - struct ibv_cq ibv_cq; - struct hfi1_cq_wc *queue; - pthread_spinlock_t lock; + struct ibv_cq ibv_cq; + struct rvt_cq_wc *queue; + pthread_spinlock_t lock; }; - /* * Receive work request queue entry. * The size of the sg_list is determined when the QP is created and stored diff --git a/providers/hfi1verbs/verbs.c b/providers/hfi1verbs/verbs.c index ff001f6..dcf6714 100644 --- a/providers/hfi1verbs/verbs.c +++ b/providers/hfi1verbs/verbs.c @@ -63,10 +63,15 @@ #include <pthread.h> #include <sys/mman.h> #include <errno.h> - #include "hfiverbs.h" #include "hfi-abi.h" +static size_t hfi1_cq_size(int cqe) +{ + return sizeof(struct rvt_cq_wc) + + sizeof(struct ib_uverbs_wc) * (cqe + 1); +} + int hfi1_query_device(struct ibv_context *context, struct ibv_device_attr *attr) { @@ -186,7 +191,7 @@ struct ibv_cq *hfi1_create_cq(struct ibv_context *context, int cqe, return NULL; } - size = sizeof(struct hfi1_cq_wc) + sizeof(struct hfi1_wc) * cqe; + size = hfi1_cq_size(cqe); cq->queue = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, context->cmd_fd, resp.offset); if ((void *) cq->queue == MAP_FAILED) { @@ -231,8 +236,7 @@ int hfi1_resize_cq(struct ibv_cq *ibcq, int cqe) memset(&resp, 0, sizeof(resp)); pthread_spin_lock(&cq->lock); /* Save the old size so we can unmmap the queue. */ - size = sizeof(struct hfi1_cq_wc) + - (sizeof(struct hfi1_wc) * cq->ibv_cq.cqe); + size = hfi1_cq_size(cq->ibv_cq.cqe); ret = ibv_cmd_resize_cq(ibcq, cqe, &cmd, sizeof cmd, &resp.ibv_resp, sizeof resp); if (ret) { @@ -240,8 +244,7 @@ int hfi1_resize_cq(struct ibv_cq *ibcq, int cqe) return ret; } (void) munmap(cq->queue, size); - size = sizeof(struct hfi1_cq_wc) + - (sizeof(struct hfi1_wc) * cq->ibv_cq.cqe); + size = hfi1_cq_size(cqe); cq->queue = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, ibcq->context->cmd_fd, resp.offset); ret = errno; @@ -269,8 +272,8 @@ int hfi1_destroy_cq(struct ibv_cq *ibcq) if (ret) return ret; - (void) munmap(cq->queue, sizeof(struct hfi1_cq_wc) + - (sizeof(struct hfi1_wc) * cq->ibv_cq.cqe)); + (void) munmap(cq->queue, hfi1_cq_size(cq->ibv_cq.cqe)); + free(cq); return 0; } @@ -288,7 +291,7 @@ int hfi1_destroy_cq_v1(struct ibv_cq *ibcq) int hfi1_poll_cq(struct ibv_cq *ibcq, int ne, struct ibv_wc *wc) { struct hfi1_cq *cq = to_icq(ibcq); - struct hfi1_cq_wc *q; + struct rvt_cq_wc *q; int npolled; uint32_t tail; @@ -300,7 +303,7 @@ int hfi1_poll_cq(struct ibv_cq *ibcq, int ne, struct ibv_wc *wc) break; /* Make sure entry is read after head index is read. */ atomic_thread_fence(memory_order_acquire); - memcpy(wc, &q->queue[tail], sizeof(*wc)); + memcpy(wc, &q->uqueue[tail], sizeof(*wc)); if (tail == cq->ibv_cq.cqe) tail = 0; else