Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
From: Leon Romanovsky <leon@kernel.org>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Doug Ledford <dledford@redhat.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
Date: Thu, 16 Jan 2020 09:32:08 +0200
Message-ID: <20200116073208.GG76932@unreal> (raw)
In-Reply-To: <20200107184019.GA20166@ziepe.ca>

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>

  reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-12  9:37 Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 01/48] RDMA/cm: Provide private data size to CM users Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 02/48] RDMA/srpt: Use private_data_len instead of hardcoded value Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 03/48] RDMA/ucma: Mask QPN to be 24 bits according to IBTA Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 04/48] RDMA/cm: Add SET/GET implementations to hide IBA wire format Leon Romanovsky
2020-01-04  2:15   ` Jason Gunthorpe
2019-12-12  9:37 ` [PATCH rdma-rc v2 05/48] RDMA/cm: Request For Communication (REQ) message definitions Leon Romanovsky
2020-01-07  1:17   ` Jason Gunthorpe
2019-12-12  9:37 ` [PATCH rdma-rc v2 06/48] RDMA/cm: Message Receipt Acknowledgment (MRA) " Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 07/48] RDMA/cm: Reject (REJ) " Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 08/48] RDMA/cm: Reply To Request for communication (REP) definitions Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 09/48] RDMA/cm: Ready To Use (RTU) definitions Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 10/48] RDMA/cm: Request For Communication Release (DREQ) definitions Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 11/48] RDMA/cm: Reply To Request For Communication Release (DREP) definitions Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 12/48] RDMA/cm: Load Alternate Path (LAP) definitions Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 13/48] RDMA/cm: Alternate Path Response (APR) message definitions Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 14/48] RDMA/cm: Service ID Resolution Request (SIDR_REQ) definitions Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 15/48] RDMA/cm: Service ID Resolution Response (SIDR_REP) definitions Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 16/48] RDMA/cm: Convert QPN and EECN to be u32 variables Leon Romanovsky
2019-12-12  9:37 ` [PATCH rdma-rc v2 17/48] RDMA/cm: Convert REQ responded resources to the new scheme Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 18/48] RDMA/cm: Convert REQ initiator depth " Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 19/48] RDMA/cm: Convert REQ remote response timeout Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 20/48] RDMA/cm: Simplify QP type to wire protocol translation Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 21/48] RDMA/cm: Convert REQ flow control Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 22/48] RDMA/cm: Convert starting PSN to be u32 variable Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 23/48] RDMA/cm: Update REQ local response timeout Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 24/48] RDMA/cm: Convert REQ retry count to use new scheme Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 25/48] RDMA/cm: Update REQ path MTU field Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 26/48] RDMA/cm: Convert REQ RNR retry timeout counter Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 27/48] RDMA/cm: Convert REQ MAX CM retries Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 28/48] RDMA/cm: Convert REQ SRQ field Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 29/48] RDMA/cm: Convert REQ flow label field Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 30/48] RDMA/cm: Convert REQ packet rate Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 31/48] RDMA/cm: Convert REQ SL fields Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 32/48] RDMA/cm: Convert REQ subnet local fields Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 33/48] RDMA/cm: Convert REQ local ack timeout Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 34/48] RDMA/cm: Convert MRA MRAed field Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 35/48] RDMA/cm: Convert MRA service timeout Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 36/48] RDMA/cm: Update REJ struct to use new scheme Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 37/48] RDMA/cm: Convert REP target ack delay field Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 38/48] RDMA/cm: Convert REP failover accepted field Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 39/48] RDMA/cm: Convert REP flow control field Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 40/48] RDMA/cm: Convert REP RNR retry count field Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 41/48] RDMA/cm: Convert REP SRQ field Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 42/48] RDMA/cm: Delete unused CM LAP functions Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 43/48] RDMA/cm: Convert LAP flow label field Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 44/48] RDMA/cm: Convert LAP fields Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 45/48] RDMA/cm: Delete unused CM ARP functions Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 46/48] RDMA/cm: Convert SIDR_REP to new scheme Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 47/48] RDMA/cm: Add Enhanced Connection Establishment (ECE) bits Leon Romanovsky
2019-12-17 12:33   ` Leon Romanovsky
2019-12-12  9:38 ` [PATCH rdma-rc v2 48/48] RDMA/cm: Convert private_date access Leon Romanovsky
2019-12-12 12:06 ` [PATCH rdma-rc v2 00/48] Organize code according to IBTA layout Leon Romanovsky
2020-01-07 18:40 ` Jason Gunthorpe
2020-01-16  7:32   ` Leon Romanovsky [this message]
2020-01-16 19:24     ` Jason Gunthorpe
2020-01-16 19:31       ` Leon Romanovsky

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200116073208.GG76932@unreal \
    --to=leon@kernel.org \
    --cc=bvanassche@acm.org \
    --cc=dledford@redhat.com \
    --cc=jgg@ziepe.ca \
    --cc=linux-rdma@vger.kernel.org \
    --cc=sean.hefty@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git