Date:   Tue, 7 Jan 2020 14:40:19 -0400
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Leon Romanovsky <leon@kernel.org>
Cc:     Doug Ledford <dledford@redhat.com>,
        Leon Romanovsky <leonro@mellanox.com>,
        RDMA mailing list <linux-rdma@vger.kernel.org>,
        Bart Van Assche <bvanassche@acm.org>,
        Sean Hefty <sean.hefty@intel.com>
Subject: Re: [PATCH rdma-rc v2 00/48] Organize code according to IBTA layout
Message-ID: <20200107184019.GA20166@ziepe.ca>
References: <20191212093830.316934-1-leon@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191212093830.316934-1-leon@kernel.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-rdma-owner@vger.kernel.org
Precedence: bulk

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