All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jason Gunthorpe <jgg@ziepe.ca>
To: Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>,
	RDMA mailing list <linux-rdma@vger.kernel.org>
Subject: Re: [PATCH rdma-next 01/43] RDMA/cm: Add naive SET/GET implementations to hide CM wire format
Date: Mon, 18 Nov 2019 09:43:12 -0400	[thread overview]
Message-ID: <20191118134312.GB2149@ziepe.ca> (raw)
In-Reply-To: <20191118130458.GD52766@unreal>

On Mon, Nov 18, 2019 at 03:04:58PM +0200, Leon Romanovsky wrote:
> On Fri, Nov 15, 2019 at 04:45:58PM -0400, Jason Gunthorpe wrote:
> > On Sun, Oct 27, 2019 at 09:05:39AM +0200, Leon Romanovsky wrote:
> >
> > >  #define IB_CM_CLASS_VERSION	2 /* IB specification 1.2 */
> > > +#define _CM_SET(p, offset, mask, value)                                        \
> > > +	({                                                                     \
> > > +		void *field = (u8 *)p + sizeof(struct ib_mad_hdr) + offset;    \
> > > +		u8 bytes =                                                     \
> > > +			DIV_ROUND_UP(__builtin_popcount(mask), BITS_PER_BYTE); \
> > > +		switch (bytes) {                                               \
> > > +		case 1: {                                                      \
> > > +			*(u8 *)field &= ~mask;                                 \
> > > +			*(u8 *)field |= FIELD_PREP(mask, value);               \
> > > +		} break;                                                       \
> > > +		case 2: {                                                      \
> > > +			u16 val = ntohs(*(__be16 *)field) & ~mask;             \
> > > +			val |= FIELD_PREP(mask, value);                        \
> > > +			*(__be16 *)field = htons(val);                         \
> > > +		} break;                                                       \
> > > +		case 3: {                                                      \
> > > +			u32 val = ntohl(*(__be32 *)field) & ~(mask << 8);      \
> > > +			val |= FIELD_PREP(mask, value) << 8;                   \
> > > +			*(__be32 *)field = htonl(val);                         \
> >
> > This doesn't work for flow label which has a 20 byte field, the <<8 is
> > just a hack to fix the 24 byte case.
> >
> > This is also some typo's:
> >
> > > + #define CM_REQ_LOCAL_EECN_OFFSET 36
> > > + #define CM_REQ_LOCAL_EECN_MASK GENMASK(24, 0)
> >
> > Should be 23, 0
> >
> > > + #define CM_REQ_PRIMARY_PACKET_RATE_OFFSET 91
> > > + #define CM_REQ_PRIMARY_PACKET_RATE_MASK GENMASK(3, 2)
> >
> > Packet rate is a 6 bit field, not a 2 bit field
> >
> > I only looked at REQ. I assume all the others have a similar error
> > rate.
> >
> > Overall, I don't like this approach. The macros are messy/buggy and
> > there isn't a clear mapping of the data in the tables to the C code.
> >
> > How about this instead:
> 
> I very liked type safety in your solution, but I think that IBA_FIELD*_LOC()
> macros add too much magic into such simple thing like spec declarations.

That 'magic' means we can just copy the spec text directly and don't
have to reprocess it by hand via different magic, which caused all the
mistakes above.

Jason

  reply	other threads:[~2019-11-18 13:43 UTC|newest]

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

Reply instructions:

You may reply publicly 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=20191118134312.GB2149@ziepe.ca \
    --to=jgg@ziepe.ca \
    --cc=dledford@redhat.com \
    --cc=leon@kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.