| Message ID | 20191212093830.316934-1-leon@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Organize code according to IBTA layout | expand |
Of course, this series to -next. I had a bug in my submission scripts. Thanks
On Thu, Dec 12, 2019 at 11:37:42AM +0200, Leon Romanovsky wrote: > RDMA/cm: Delete unused CM LAP functions > RDMA/cm: Delete unused CM ARP functions These are applied to for-next > Leon Romanovsky (48): > RDMA/cm: Provide private data size to CM users > RDMA/srpt: Use private_data_len instead of hardcoded value > RDMA/ucma: Mask QPN to be 24 bits according to IBTA > RDMA/cm: Add SET/GET implementations to hide IBA wire format > RDMA/cm: Request For Communication (REQ) message definitions > RDMA/cm: Message Receipt Acknowledgment (MRA) message definitions > RDMA/cm: Reject (REJ) message definitions > RDMA/cm: Reply To Request for communication (REP) definitions > RDMA/cm: Ready To Use (RTU) definitions > RDMA/cm: Request For Communication Release (DREQ) definitions > RDMA/cm: Reply To Request For Communication Release (DREP) definitions > RDMA/cm: Load Alternate Path (LAP) definitions > RDMA/cm: Alternate Path Response (APR) message definitions > RDMA/cm: Service ID Resolution Request (SIDR_REQ) definitions > RDMA/cm: Service ID Resolution Response (SIDR_REP) definitions > RDMA/cm: Convert QPN and EECN to be u32 variables > RDMA/cm: Convert REQ responded resources to the new scheme > RDMA/cm: Convert REQ initiator depth to the new scheme > RDMA/cm: Convert REQ remote response timeout > RDMA/cm: Simplify QP type to wire protocol translation > RDMA/cm: Convert REQ flow control > RDMA/cm: Convert starting PSN to be u32 variable > RDMA/cm: Update REQ local response timeout > RDMA/cm: Convert REQ retry count to use new scheme > RDMA/cm: Update REQ path MTU field > RDMA/cm: Convert REQ RNR retry timeout counter > RDMA/cm: Convert REQ MAX CM retries > RDMA/cm: Convert REQ SRQ field > RDMA/cm: Convert REQ flow label field > RDMA/cm: Convert REQ packet rate > RDMA/cm: Convert REQ SL fields > RDMA/cm: Convert REQ subnet local fields > RDMA/cm: Convert REQ local ack timeout > RDMA/cm: Convert MRA MRAed field > RDMA/cm: Convert MRA service timeout > RDMA/cm: Update REJ struct to use new scheme > RDMA/cm: Convert REP target ack delay field > RDMA/cm: Convert REP failover accepted field > RDMA/cm: Convert REP flow control field > RDMA/cm: Convert REP RNR retry count field > RDMA/cm: Convert REP SRQ field > RDMA/cm: Convert LAP flow label field > RDMA/cm: Convert LAP fields > RDMA/cm: Convert SIDR_REP to new scheme > RDMA/cm: Add Enhanced Connection Establishment (ECE) bits > RDMA/cm: Convert private_date access I spent a long, long time looking at this. Far too long. The series is too big, and the patches make too many changes all at once. There are also many problems with the IBA_GET/etc macros I gave you. Finally, I didn't like that it only did half the job and still left the old structs around. I fixed it all up, and put it here: https://github.com/jgunthorpe/linux/commits/cm_rework I originaly started by just writing out the IBA_CHECK things in the first patch. This showed that the IBA_ acessors were not working right (I fixed all those too). At the end of that exercise I had full confidence that the new macros and the field descriptors were OK. When I started to look at the actual conversion patches and doing the missing ones, I realized this whole thing was trivially done via spatch. So I made a script that took the proven mapping of new names to old names and had it code gen a spatch script which then was applied. I split the spatch rules into 4 patches bases on 'kind of thing' being converted. The first two can be diffed against your series. I didn't observe any problems, so the conversion was probably good. However, it was hard to tell as there was lots of functional changes mixed in your series, like dropping more BE's and what not. The last two complete the work and convert all the loose structure members. The final one deletes most of cm_msgs.h I have a pretty high confidence in the spatch process and the input markup. But I didn't run sparse or test it. While this does not do everything your series did, it gobbles up all the high LOC stuff and the remaining things like dropping more of the be's are best done as smaller followup patches which can be applied right away. The full diffstat is ridiculous: 5 files changed, 852 insertions(+), 1253 deletions(-) Please check the revised series and let me know. Thanks, Jason
On Tue, Jan 07, 2020 at 02:40:19PM -0400, Jason Gunthorpe wrote: > On Thu, Dec 12, 2019 at 11:37:42AM +0200, Leon Romanovsky wrote: > > RDMA/cm: Delete unused CM LAP functions > > RDMA/cm: Delete unused CM ARP functions > > These are applied to for-next > > > Leon Romanovsky (48): > > RDMA/cm: Provide private data size to CM users > > RDMA/srpt: Use private_data_len instead of hardcoded value > > RDMA/ucma: Mask QPN to be 24 bits according to IBTA > > RDMA/cm: Add SET/GET implementations to hide IBA wire format > > RDMA/cm: Request For Communication (REQ) message definitions > > RDMA/cm: Message Receipt Acknowledgment (MRA) message definitions > > RDMA/cm: Reject (REJ) message definitions > > RDMA/cm: Reply To Request for communication (REP) definitions > > RDMA/cm: Ready To Use (RTU) definitions > > RDMA/cm: Request For Communication Release (DREQ) definitions > > RDMA/cm: Reply To Request For Communication Release (DREP) definitions > > RDMA/cm: Load Alternate Path (LAP) definitions > > RDMA/cm: Alternate Path Response (APR) message definitions > > RDMA/cm: Service ID Resolution Request (SIDR_REQ) definitions > > RDMA/cm: Service ID Resolution Response (SIDR_REP) definitions > > RDMA/cm: Convert QPN and EECN to be u32 variables > > RDMA/cm: Convert REQ responded resources to the new scheme > > RDMA/cm: Convert REQ initiator depth to the new scheme > > RDMA/cm: Convert REQ remote response timeout > > RDMA/cm: Simplify QP type to wire protocol translation > > RDMA/cm: Convert REQ flow control > > RDMA/cm: Convert starting PSN to be u32 variable > > RDMA/cm: Update REQ local response timeout > > RDMA/cm: Convert REQ retry count to use new scheme > > RDMA/cm: Update REQ path MTU field > > RDMA/cm: Convert REQ RNR retry timeout counter > > RDMA/cm: Convert REQ MAX CM retries > > RDMA/cm: Convert REQ SRQ field > > RDMA/cm: Convert REQ flow label field > > RDMA/cm: Convert REQ packet rate > > RDMA/cm: Convert REQ SL fields > > RDMA/cm: Convert REQ subnet local fields > > RDMA/cm: Convert REQ local ack timeout > > RDMA/cm: Convert MRA MRAed field > > RDMA/cm: Convert MRA service timeout > > RDMA/cm: Update REJ struct to use new scheme > > RDMA/cm: Convert REP target ack delay field > > RDMA/cm: Convert REP failover accepted field > > RDMA/cm: Convert REP flow control field > > RDMA/cm: Convert REP RNR retry count field > > RDMA/cm: Convert REP SRQ field > > RDMA/cm: Convert LAP flow label field > > RDMA/cm: Convert LAP fields > > RDMA/cm: Convert SIDR_REP to new scheme > > RDMA/cm: Add Enhanced Connection Establishment (ECE) bits > > RDMA/cm: Convert private_date access > > I spent a long, long time looking at this. Far too long. > > The series is too big, and the patches make too many changes all at > once. There are also many problems with the IBA_GET/etc macros I gave > you. Finally, I didn't like that it only did half the job and still > left the old structs around. > > I fixed it all up, and put it here: > > https://github.com/jgunthorpe/linux/commits/cm_rework > > I originaly started by just writing out the IBA_CHECK things in the > first patch. This showed that the IBA_ acessors were not working right > (I fixed all those too). At the end of that exercise I had full > confidence that the new macros and the field descriptors were OK. > > When I started to look at the actual conversion patches and doing the > missing ones, I realized this whole thing was trivially done via > spatch. So I made a script that took the proven mapping of new names > to old names and had it code gen a spatch script which then was > applied. > > I split the spatch rules into 4 patches bases on 'kind of thing' being > converted. > > The first two can be diffed against your series. I didn't observe any > problems, so the conversion was probably good. However, it was hard to > tell as there was lots of functional changes mixed in your series, > like dropping more BE's and what not. > > The last two complete the work and convert all the loose structure > members. The final one deletes most of cm_msgs.h > > I have a pretty high confidence in the spatch process and the input > markup. But I didn't run sparse or test it. > > While this does not do everything your series did, it gobbles up all > the high LOC stuff and the remaining things like dropping more of the > be's are best done as smaller followup patches which can be applied > right away. > > The full diffstat is ridiculous: > 5 files changed, 852 insertions(+), 1253 deletions(-) > > Please check the revised series and let me know. Hi Jason, We tested the series and I reviewed it on github, everything looks amazing, and I have only three nitpicks. 1. "exta" -> "extra" 2. IMHO, you don't need to include your selftest in final patches, because the whole series is going to be accepted and that code will be added and deleted at the same time. Especially printk part. 3. Copyright needs to be 2020 Thanks, Tested-by: Leon Romanovsky <leonro@mellanox.com> Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On Thu, Jan 16, 2020 at 09:32:08AM +0200, Leon Romanovsky wrote: > 2. IMHO, you don't need to include your selftest in final patches, because > the whole series is going to be accepted and that code will be added and > deleted at the same time. Especially printk part. I like seeing the tests. For a patch like this, which is so tedious to review, it makes the review a check of the tests, a check of the spatch and some spot checks of the transformations. Since it is a small number of lines, and it is much easier than sending the tests separately, it felt reasonable to leave them in the history. Will you be able to send the _be removal conversions you had done on top of this? I didn't show it, but all the private_data_len, etc should be some generic IBA_NUM_BYTES() accessor like get/set instead of more #defines. Jason
On Thu, Jan 16, 2020 at 03:24:04PM -0400, Jason Gunthorpe wrote: > On Thu, Jan 16, 2020 at 09:32:08AM +0200, Leon Romanovsky wrote: > > > 2. IMHO, you don't need to include your selftest in final patches, because > > the whole series is going to be accepted and that code will be added and > > deleted at the same time. Especially printk part. > > I like seeing the tests. For a patch like this, which is so tedious to > review, it makes the review a check of the tests, a check of the > spatch and some spot checks of the transformations. > > Since it is a small number of lines, and it is much easier than > sending the tests separately, it felt reasonable to leave them in the > history. > > Will you be able to send the _be removal conversions you had done on > top of this? It will time to make rebase, but I'll do. > > I didn't show it, but all the private_data_len, etc should be some > generic IBA_NUM_BYTES() accessor like get/set instead of more #defines. > > Jason
From: Leon Romanovsky <leonro@mellanox.com> Changelog: v1->v2: https://lore.kernel.org/linux-rdma/20191121181313.129430-1-leon@kernel.org * Added forgotten CM_FIELD64_LOC(). v0->v1: https://lore.kernel.org/linux-rdma/20191027070621.11711-1-leon@kernel.org * Used Jason's macros as a basis for all get/set operation for wire protocol. * Fixed wrong offsets. * Grouped all CM related patches in one patchset bomb. ---------------------------------------------------------------------- Hi, This series continues already started task to clean up CM related code. Over the years, the IB/core gained a number of anti-patterns which led to mistakes. First and most distracting is spread of hardware specification types (e.g. __beXX) to the core logic. Second, endless copy/paste to access IBTA binary blobs, which made any IBTA extensions not an easy task. In this series, we add Enhance Connection Establishment bits which were added recently to IBTA and will continue to convert rest of the CM code to propose macros by eliminating __beXX variables from core code. All IBTA CM declarations are places into new header file: include/rdma/ibta_vol1_c12.h and the idea that every spec chapter will have separate header file, so we will see immediately the relations between declarations and values. Thanks BTW, 1. The whole area near private_data looks sketchy to me and needs separate cleanup. 2. I know that it is more than 15 patches, but they are small and self-contained. Leon Romanovsky (48): RDMA/cm: Provide private data size to CM users RDMA/srpt: Use private_data_len instead of hardcoded value RDMA/ucma: Mask QPN to be 24 bits according to IBTA RDMA/cm: Add SET/GET implementations to hide IBA wire format RDMA/cm: Request For Communication (REQ) message definitions RDMA/cm: Message Receipt Acknowledgment (MRA) message definitions RDMA/cm: Reject (REJ) message definitions RDMA/cm: Reply To Request for communication (REP) definitions RDMA/cm: Ready To Use (RTU) definitions RDMA/cm: Request For Communication Release (DREQ) definitions RDMA/cm: Reply To Request For Communication Release (DREP) definitions RDMA/cm: Load Alternate Path (LAP) definitions RDMA/cm: Alternate Path Response (APR) message definitions RDMA/cm: Service ID Resolution Request (SIDR_REQ) definitions RDMA/cm: Service ID Resolution Response (SIDR_REP) definitions RDMA/cm: Convert QPN and EECN to be u32 variables RDMA/cm: Convert REQ responded resources to the new scheme RDMA/cm: Convert REQ initiator depth to the new scheme RDMA/cm: Convert REQ remote response timeout RDMA/cm: Simplify QP type to wire protocol translation RDMA/cm: Convert REQ flow control RDMA/cm: Convert starting PSN to be u32 variable RDMA/cm: Update REQ local response timeout RDMA/cm: Convert REQ retry count to use new scheme RDMA/cm: Update REQ path MTU field RDMA/cm: Convert REQ RNR retry timeout counter RDMA/cm: Convert REQ MAX CM retries RDMA/cm: Convert REQ SRQ field RDMA/cm: Convert REQ flow label field RDMA/cm: Convert REQ packet rate RDMA/cm: Convert REQ SL fields RDMA/cm: Convert REQ subnet local fields RDMA/cm: Convert REQ local ack timeout RDMA/cm: Convert MRA MRAed field RDMA/cm: Convert MRA service timeout RDMA/cm: Update REJ struct to use new scheme RDMA/cm: Convert REP target ack delay field RDMA/cm: Convert REP failover accepted field RDMA/cm: Convert REP flow control field RDMA/cm: Convert REP RNR retry count field RDMA/cm: Convert REP SRQ field RDMA/cm: Delete unused CM LAP functions RDMA/cm: Convert LAP flow label field RDMA/cm: Convert LAP fields RDMA/cm: Delete unused CM ARP functions RDMA/cm: Convert SIDR_REP to new scheme RDMA/cm: Add Enhanced Connection Establishment (ECE) bits RDMA/cm: Convert private_date access drivers/infiniband/core/cm.c | 554 ++++++++++-------------- drivers/infiniband/core/cm_msgs.h | 600 +------------------------- drivers/infiniband/core/cma.c | 11 +- drivers/infiniband/core/ucma.c | 2 +- drivers/infiniband/ulp/srpt/ib_srpt.c | 2 +- include/rdma/ib_cm.h | 55 +-- include/rdma/iba.h | 138 ++++++ include/rdma/ibta_vol1_c12.h | 211 +++++++++ 8 files changed, 599 insertions(+), 974 deletions(-) create mode 100644 include/rdma/iba.h create mode 100644 include/rdma/ibta_vol1_c12.h -- 2.20.1