| Message ID | 552BB85D.7010400@profitbricks.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, Apr 13, 2015 at 02:36:45PM +0200, Michael Wang wrote: > We have finished introducing the cap_XX(), and raw helper rdma_ib_or_iboe() > is no longer necessary, thus clean it up. So, the net result is not looking too bad, but I'm confused about the structure of this series. Why introduce query_transport early on? Why is the patch series going through this progression most lines? - if (rdma_port_get_link_layer(device, port_num) == IB_LINK_LAYER_INFINIBAND) { + if (rdma_tech_ib(device, port_num)) { + if (cap_ib_smi(device, port_num)) { This would be a lot shorter and simpler to look at if the cap's were introduced first, then their implementation finalized. I thought we agreed Doug's bitmask plan should be the final destination for this series, so this progress seems even stranger? I would be very happy to see a patch that adds cap_ib_smi to the current tree and states 'This patch is tested to have no change on the binary compilation results' Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/13/2015 10:33 PM, Jason Gunthorpe wrote: > On Mon, Apr 13, 2015 at 02:36:45PM +0200, Michael Wang wrote: >> We have finished introducing the cap_XX(), and raw helper rdma_ib_or_iboe() >> is no longer necessary, thus clean it up. > > So, the net result is not looking too bad, but I'm confused about the > structure of this series. > > Why introduce query_transport early on? This won't be erased until bitmask introduced, at this moment it's the basic method for helpers to acquire port transport from device. Sure we can still use the legacy method but IMHO this abstraction will be more readable for internal reforming, it's like 'mapping from tech to bits' VS 'mapping from transport and link layer to bits'. > > Why is the patch series going through this progression most lines? > > - if (rdma_port_get_link_layer(device, port_num) == IB_LINK_LAYER_INFINIBAND) { > + if (rdma_tech_ib(device, port_num)) { This is mostly focus on the reforming on logical. > + if (cap_ib_smi(device, port_num)) { This focus on the description and semantic, won't contain logical reform , just replace the helper. I want this way to help us focus on the different main point during the review :-) > > This would be a lot shorter and simpler to look at if the cap's were > introduced first, then their implementation finalized. > > I thought we agreed Doug's bitmask plan should be the final > destination for this series, so this progress seems even stranger? > > I would be very happy to see a patch that adds cap_ib_smi to the > current tree and states 'This patch is tested to have no change on the > binary compilation results' There are too much reform there (per-dev to per-port), I guess the binary will changed more or less anyway... BTW, I may misunderstanding your point on "Re: [PATCH v2 03/17]": > I would prefer to see these changes in control flow as dedicated > patches, at the top of your patch stack. > For this kind of work a patch should be mechanical changes only, it is > easier to review that way. > Same comment applies throughout. I thought you prefer introducing cap_XX() based on the reforming... But anyway, please let me know if this really bothered the review :-) Regards, Michael Wang > > Jason > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 14, 2015 at 11:13:03AM +0200, Michael Wang wrote: > > I would be very happy to see a patch that adds cap_ib_smi to the > > current tree and states 'This patch is tested to have no change on the > > binary compilation results' > > There are too much reform there (per-dev to per-port), I guess the binary > will changed more or less anyway... I think this patch series is huge, and everytime someone new looks at it small functional errors seem to pop up.. Doing something to reduce the review surface would be really helpful here. Not changing the same line twice, using tools too perform these transforms and then assert the patch is a NOP because .. tools. Some other idea? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] > Sent: Thursday, April 16, 2015 11:43 AM > To: Michael Wang > Cc: Roland Dreier; Sean Hefty; Hal Rosenstock; linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org; Tom Tucker; Steve Wise; > Hoang-Nam Nguyen; Christoph Raisch; Mike Marciniszyn; Eli Cohen; Faisal Latif; Jack Morgenstein; Or Gerlitz; Haggai Eran; Ira Weiny; > Tom Talpey; Doug Ledford > Subject: Re: [PATCH v3 27/28] IB/Verbs: Clean up rdma_ib_or_iboe() > > On Tue, Apr 14, 2015 at 11:13:03AM +0200, Michael Wang wrote: > > > > I would be very happy to see a patch that adds cap_ib_smi to the > > > current tree and states 'This patch is tested to have no change on the > > > binary compilation results' > > > > There are too much reform there (per-dev to per-port), I guess the binary > > will changed more or less anyway... > > I think this patch series is huge, and everytime someone new looks at > it small functional errors seem to pop up.. > > Doing something to reduce the review surface would be really helpful > here. Not changing the same line twice, using tools too perform these > transforms and then assert the patch is a NOP because .. tools. Some > other idea? > Don't try and change everything in one giant series. Just do some changes this cycle (keep it at < 8 or 10 patches), and do more later... -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/16/2015 06:43 PM, Jason Gunthorpe wrote: > On Tue, Apr 14, 2015 at 11:13:03AM +0200, Michael Wang wrote: > >>> I would be very happy to see a patch that adds cap_ib_smi to the >>> current tree and states 'This patch is tested to have no change on the >>> binary compilation results' >> >> There are too much reform there (per-dev to per-port), I guess the binary >> will changed more or less anyway... > > I think this patch series is huge, and everytime someone new looks at > it small functional errors seem to pop up.. This is a big changing after all :-P As Doug suggested at very beginning, all these changing are necessary in order to eliminate the usage of old inferring method, then we will have a clean stage for next reform. And since it's big, I tried to classified them according to logical, to help us review more easily, I'm not sure but compress the series may increasing the difficulty of reviewing... > > Doing something to reduce the review surface would be really helpful > here. Not changing the same line twice, using tools too perform these > transforms and then assert the patch is a NOP because .. tools. Some > other idea? Actually the main reform work finished in 1#~15#, the rest are just introducing cap_XX which we only need to check the description and usage, thus I'd like to suggest we focus on reviewing 1#~15#, after all, the rest won't introducing Bug and we can edit them at any time :-P Frankly speaking I think it's a good thing that we locate errors at this moment, whenever someone find issues, that means the patch has been reviewed thoroughly, I think may be just few more version, this series will become stable ;-) Regards, Michael Wang > > Jason > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/16/2015 08:07 PM, Steve Wise wrote: > > >> -----Original Message----- >> From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] >> Sent: Thursday, April 16, 2015 11:43 AM >> To: Michael Wang >> Cc: Roland Dreier; Sean Hefty; Hal Rosenstock; linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org; Tom Tucker; Steve Wise; >> Hoang-Nam Nguyen; Christoph Raisch; Mike Marciniszyn; Eli Cohen; Faisal Latif; Jack Morgenstein; Or Gerlitz; Haggai Eran; Ira > Weiny; >> Tom Talpey; Doug Ledford >> Subject: Re: [PATCH v3 27/28] IB/Verbs: Clean up rdma_ib_or_iboe() >> >> On Tue, Apr 14, 2015 at 11:13:03AM +0200, Michael Wang wrote: >> >>>> I would be very happy to see a patch that adds cap_ib_smi to the >>>> current tree and states 'This patch is tested to have no change on the >>>> binary compilation results' >>> >>> There are too much reform there (per-dev to per-port), I guess the binary >>> will changed more or less anyway... >> >> I think this patch series is huge, and everytime someone new looks at >> it small functional errors seem to pop up.. >> >> Doing something to reduce the review surface would be really helpful >> here. Not changing the same line twice, using tools too perform these >> transforms and then assert the patch is a NOP because .. tools. Some >> other idea? >> > > Don't try and change everything in one giant series. Just do some changes this cycle (keep it at < 8 or 10 patches), and do more > later... Actually only 1#~15# related to logical reform, rest are just replacement :-) Me too would like to stop introducing new stuff at this moment, and focus on the improvement of what we have already settled down. Regards, Michael Wang > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 78a5cdb2..9f6b88e 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -1766,13 +1766,6 @@ static inline int rdma_tech_iwarp(struct ib_device *device, u8 port_num) == RDMA_TRANSPORT_IWARP; } -static inline int rdma_ib_or_iboe(struct ib_device *device, u8 port_num) -{ - enum rdma_transport_type tp = device->query_transport(device, port_num); - - return (tp == RDMA_TRANSPORT_IB || tp == RDMA_TRANSPORT_IBOE); -} - /** * cap_ib_mad - Check if the port of device has the capability Infiniband * Management Datagrams. @@ -1785,7 +1778,9 @@ static inline int rdma_ib_or_iboe(struct ib_device *device, u8 port_num) */ static inline int cap_ib_mad(struct ib_device *device, u8 port_num) { - return rdma_ib_or_iboe(device, port_num); + enum rdma_transport_type tp = device->query_transport(device, port_num); + + return (tp == RDMA_TRANSPORT_IB || tp == RDMA_TRANSPORT_IBOE); } /** @@ -1815,7 +1810,9 @@ static inline int cap_ib_smi(struct ib_device *device, u8 port_num) */ static inline int cap_ib_cm(struct ib_device *device, u8 port_num) { - return rdma_ib_or_iboe(device, port_num); + enum rdma_transport_type tp = device->query_transport(device, port_num); + + return (tp == RDMA_TRANSPORT_IB || tp == RDMA_TRANSPORT_IBOE); } /** @@ -1890,7 +1887,9 @@ static inline int cap_ipoib(struct ib_device *device, u8 port_num) */ static inline int cap_af_ib(struct ib_device *device, u8 port_num) { - return rdma_ib_or_iboe(device, port_num); + enum rdma_transport_type tp = device->query_transport(device, port_num); + + return (tp == RDMA_TRANSPORT_IB || tp == RDMA_TRANSPORT_IBOE); } /** diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c index a09b7a1..8af6f92 100644 --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c @@ -987,8 +987,10 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt) */ if (!rdma_tech_iwarp(newxprt->sc_cm_id->device, newxprt->sc_cm_id->port_num) && - !rdma_ib_or_iboe(newxprt->sc_cm_id->device, - newxprt->sc_cm_id->port_num)) + !rdma_tech_ib(newxprt->sc_cm_id->device, + newxprt->sc_cm_id->port_num) && + !rdma_tech_iboe(newxprt->sc_cm_id->device, + newxprt->sc_cm_id->port_num)) goto errout; if (!(newxprt->sc_dev_caps & SVCRDMA_DEVCAP_FAST_REG) ||
We have finished introducing the cap_XX(), and raw helper rdma_ib_or_iboe() is no longer necessary, thus clean it up. Cc: Steve Wise <swise@opengridcomputing.com> Cc: Tom Talpey <tom@talpey.com> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Cc: Doug Ledford <dledford@redhat.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Sean Hefty <sean.hefty@intel.com> Signed-off-by: Michael Wang <yun.wang@profitbricks.com> --- include/rdma/ib_verbs.h | 19 +++++++++---------- net/sunrpc/xprtrdma/svc_rdma_transport.c | 6 ++++-- 2 files changed, 13 insertions(+), 12 deletions(-)