Re: [PATCH] IB/mad: Clean up ib_find_send_mad to use only the ib_mad_hdr

Re: [PATCH] IB/mad: Clean up ib_find_send_mad to use only the ib_mad_hdr

[Or Gerlitz]

On Thu, May 14, 2015 at 2:20 AM,  <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> From: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> ib_find_send_mad only needs access to the MAD header.  Change the MAD cast.
>
> Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

shorter and clearer subject line please, e.g

IB/mad: Clean up ib_find_send_mad to use only the MAD header

Also, please try to send patches in series and not as both singles and
series with multiple series flowing in the same time and touching the
same module. It's hard for both reviewers and maintainers to follow.

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/mad: Clean up ib_find_send_mad to use only the ib_mad_hdr

[ira.weiny]

On Thu, May 14, 2015 at 10:28:45AM +0300, Or Gerlitz wrote:
> On Thu, May 14, 2015 at 2:20 AM,  <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> > From: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >
> > ib_find_send_mad only needs access to the MAD header.  Change the MAD cast.
> >
> > Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> shorter and clearer subject line please, e.g

Sorry, I'll try to keep them brief.

> 
> IB/mad: Clean up ib_find_send_mad to use only the MAD header
> 
> Also, please try to send patches in series and not as both singles and
> series with multiple series flowing in the same time and touching the
> same module. It's hard for both reviewers and maintainers to follow.

I don't understand this statement.

This stands on its own based on Dougs "doug/to-be-rebased/for-4.2"
branch.

Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/mad: Clean up ib_find_send_mad to use only the ib_mad_hdr

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 3802 bytes --]

On Tue, 2015-05-19 at 07:44 +0300, Or Gerlitz wrote:
> On 5/14/2015 8:35 PM, ira.weiny wrote:
> > On Thu, May 14, 2015 at 10:28:45AM +0300, Or Gerlitz wrote:
> >> >On Thu, May 14, 2015 at 2:20 AM,<ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>  wrote:
> >>> > >From: Ira Weiny<ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>> > >
> >>> > >ib_find_send_mad only needs access to the MAD header.  Change the MAD cast.
> >>> > >
> >>> > >Signed-off-by: Ira Weiny<ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >> >
> >> >shorter and clearer subject line please, e.g
> > Sorry, I'll try to keep them brief.
> 
> this isn't only on being brief,  but rather of being high level and not 
> carrying too many names of functions and variables, looking on the 
> titles of your 4.2  patches, bunch of them are too long and down to 
> details instead
> of capturing the proposed change in higher level:
> 
> 6f430b IB/mad: Add const qualifiers to query only functions
Ok
> 67b3b05 IB/mad: Clean up rcv_has_same_class
Ok
> fcd6ebf IB/mad: Change ib_response_mad signature to take const 
> ib_mad_hdr rather than ib_mad
IB/mad: Change ib_response_mad signature arguments
> 3b53339 IB/mad: Change validate_mad signature to take const ib_mad_hdr 
> rather than ib_mad
IB/mad: Change validate_mad signature arguments
> b78d28a IB/mad: Clean up comments in smi.c
Ok
> c597eee IB/mad: Rename is_data_mad to is_rmpp_data_mad
Ok
> 81836bc IB/core: Change rdma_protcol_iboe to roce
Ok
> 6f9ab33 IB/core: Convert management helpers to core capability bits
IB/core: Convert core to use bitfield for caps
> 23ceb15 IB/core: Formalize the creation of immutable per port data 
> within the ib_device object
IB/core: Add per port immutable struct to ib_device
> cb3ed7b IB/user_mad: Fix bug in ib_umad_remove_one when rdma_cap_ib_mad 
> implementation changed
IB/user_mad: Fix buggy usage of port index
> 42997f2 IB/user_mad: Remove local start/end port variable and use the 
> new common functions
IB/user_mad: Use new start/end port functions
> 0cf18d7 IB/core: Create common start/end port functions
Ok
> 
> Maybe submit them all again with more proper commit titles? it's not too 
> late, Doug, what's your
> thinking here?

I try to stick within the recommended length of git log subject as
indicated to me by vim when I'm editing my commit messages.  Whatever
happens to fit inside that limit (or at least close to it, I'm not a
real stickler on it) I go with unless you really need to capture some
important detail in the subject.

However, there's no need to resubmit these.  I can always edit the
subjects in place.  Ira are you OK with the changes I made above?

Just to be clear though, to my knowledge, the whole reason for this is
so that git log --oneline will print out pretty output with one commit
per line.  That means the maximum length of these subject messages is
limited by standard terminal width (80 columns) and the selected git
commit hash abbreviation length (which has varied due to the increasing
number of collisions in git hashes and is now set at a standard of 12
characters).  So the proper length 3 years ago and the proper length
today are not the same thing.  What's more, git merge commits almost
*never* fit on a single line due to the length of git urls.  As a
result, our pretty --oneline output is already marred upstream by all
those merges.  So do I prefer commit messages that fit on one line?
Yes.  Will I demand things be resent if they don't?  No.  We've already
lost the war because of those merge commits, winning individual battles
now is nice, but not absolutely necessary.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] IB/mad: Clean up ib_find_send_mad to use only the ib_mad_hdr

[ira.weiny]

> > 
> > 6f430b IB/mad: Add const qualifiers to query only functions
> Ok
> > 67b3b05 IB/mad: Clean up rcv_has_same_class
> Ok
> > fcd6ebf IB/mad: Change ib_response_mad signature to take const 
> > ib_mad_hdr rather than ib_mad
> IB/mad: Change ib_response_mad signature arguments
> > 3b53339 IB/mad: Change validate_mad signature to take const ib_mad_hdr 
> > rather than ib_mad
> IB/mad: Change validate_mad signature arguments
> > b78d28a IB/mad: Clean up comments in smi.c
> Ok
> > c597eee IB/mad: Rename is_data_mad to is_rmpp_data_mad
> Ok
> > 81836bc IB/core: Change rdma_protcol_iboe to roce
> Ok
> > 6f9ab33 IB/core: Convert management helpers to core capability bits
> IB/core: Convert core to use bitfield for caps
> > 23ceb15 IB/core: Formalize the creation of immutable per port data 
> > within the ib_device object
> IB/core: Add per port immutable struct to ib_device
> > cb3ed7b IB/user_mad: Fix bug in ib_umad_remove_one when rdma_cap_ib_mad 
> > implementation changed
> IB/user_mad: Fix buggy usage of port index
> > 42997f2 IB/user_mad: Remove local start/end port variable and use the 
> > new common functions
> IB/user_mad: Use new start/end port functions
> > 0cf18d7 IB/core: Create common start/end port functions
> Ok
> > 
> > Maybe submit them all again with more proper commit titles? it's not too 
> > late, Doug, what's your
> > thinking here?
> 
> I try to stick within the recommended length of git log subject as
> indicated to me by vim when I'm editing my commit messages.  Whatever
> happens to fit inside that limit (or at least close to it, I'm not a
> real stickler on it) I go with unless you really need to capture some
> important detail in the subject.
> 
> However, there's no need to resubmit these.  I can always edit the
> subjects in place.  Ira are you OK with the changes I made above?

Yes thank you.  I will do better in the future.

Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/mad: Clean up ib_find_send_mad to use only the ib_mad_hdr

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2626 bytes --]

On Tue, 2015-05-19 at 14:27 -0400, ira.weiny wrote:
> > > 
> > > 6f430b IB/mad: Add const qualifiers to query only functions
> > Ok
> > > 67b3b05 IB/mad: Clean up rcv_has_same_class
> > Ok
> > > fcd6ebf IB/mad: Change ib_response_mad signature to take const 
> > > ib_mad_hdr rather than ib_mad
> > IB/mad: Change ib_response_mad signature arguments
> > > 3b53339 IB/mad: Change validate_mad signature to take const ib_mad_hdr 
> > > rather than ib_mad
> > IB/mad: Change validate_mad signature arguments
> > > b78d28a IB/mad: Clean up comments in smi.c
> > Ok
> > > c597eee IB/mad: Rename is_data_mad to is_rmpp_data_mad
> > Ok
> > > 81836bc IB/core: Change rdma_protcol_iboe to roce
> > Ok
> > > 6f9ab33 IB/core: Convert management helpers to core capability bits
> > IB/core: Convert core to use bitfield for caps
> > > 23ceb15 IB/core: Formalize the creation of immutable per port data 
> > > within the ib_device object
> > IB/core: Add per port immutable struct to ib_device
> > > cb3ed7b IB/user_mad: Fix bug in ib_umad_remove_one when rdma_cap_ib_mad 
> > > implementation changed
> > IB/user_mad: Fix buggy usage of port index
> > > 42997f2 IB/user_mad: Remove local start/end port variable and use the 
> > > new common functions
> > IB/user_mad: Use new start/end port functions
> > > 0cf18d7 IB/core: Create common start/end port functions
> > Ok
> > > 
> > > Maybe submit them all again with more proper commit titles? it's not too 
> > > late, Doug, what's your
> > > thinking here?
> > 
> > I try to stick within the recommended length of git log subject as
> > indicated to me by vim when I'm editing my commit messages.  Whatever
> > happens to fit inside that limit (or at least close to it, I'm not a
> > real stickler on it) I go with unless you really need to capture some
> > important detail in the subject.
> > 
> > However, there's no need to resubmit these.  I can always edit the
> > subjects in place.  Ira are you OK with the changes I made above?
> 
> Yes thank you.  I will do better in the future.

No worries.  They're all edited.  I changed just a couple of the more
detailed descriptions too, just because once the subject was simplified
the body deserved a little more explanation.

> 
> Ira
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] IB/mad: Clean up ib_find_send_mad to use only the ib_mad_hdr

[Or Gerlitz]

On Tue, May 19, 2015 at 8:01 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> I try to stick within the recommended length of git log subject as
> indicated to me by vim when I'm editing my commit messages.  Whatever
> happens to fit inside that limit (or at least close to it, I'm not a
> real stickler on it) I go with unless you really need to capture some
> important detail in the subject.
>
> However, there's no need to resubmit these.  I can always edit the
> subjects in place.  Ira are you OK with the changes I made above?

Doug,

Thanks for fixing these commit titles, looks much better.

The issue I have tried to put on the table goes way beyond this or
that limitation on commit titles lengths. It's about being short,
clear and strict, avoid lengthy names of variables and functions which
add nothing to proper maintenance. Ira, in your next commits, maybe
look a bit around, e.g on commit titles of patches made by Sean to
similar areas (mad, sa client, cm, cma and such), and try to get the
spirit, happy upstreaming...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] IB/mad: Clean up ib_find_send_mad to use only the ib_mad_hdr

[ira.weiny-ral2JQCrhuEAvxtiuMwx3w]

From: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

ib_find_send_mad only needs access to the MAD header.  Change the MAD cast.

Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/core/mad.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 600af266838c..deefe5df9697 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -1815,18 +1815,18 @@ ib_find_send_mad(const struct ib_mad_agent_private *mad_agent_priv,
 		 const struct ib_mad_recv_wc *wc)
 {
 	struct ib_mad_send_wr_private *wr;
-	struct ib_mad *mad;
+	struct ib_mad_hdr *mad_hdr;
 
-	mad = (struct ib_mad *)wc->recv_buf.mad;
+	mad_hdr = (struct ib_mad_hdr *)wc->recv_buf.mad;
 
 	list_for_each_entry(wr, &mad_agent_priv->wait_list, agent_list) {
-		if ((wr->tid == mad->mad_hdr.tid) &&
+		if ((wr->tid == mad_hdr->tid) &&
 		    rcv_has_same_class(wr, wc) &&
 		    /*
 		     * Don't check GID for direct routed MADs.
 		     * These might have permissive LIDs.
 		     */
-		    (is_direct(wc->recv_buf.mad->mad_hdr.mgmt_class) ||
+		    (is_direct(mad_hdr->mgmt_class) ||
 		     rcv_has_same_gid(mad_agent_priv, wr, wc)))
 			return (wr->status == IB_WC_SUCCESS) ? wr : NULL;
 	}
@@ -1837,14 +1837,14 @@ ib_find_send_mad(const struct ib_mad_agent_private *mad_agent_priv,
 	 */
 	list_for_each_entry(wr, &mad_agent_priv->send_list, agent_list) {
 		if (is_rmpp_data_mad(mad_agent_priv, wr->send_buf.mad) &&
-		    wr->tid == mad->mad_hdr.tid &&
+		    wr->tid == mad_hdr->tid &&
 		    wr->timeout &&
 		    rcv_has_same_class(wr, wc) &&
 		    /*
 		     * Don't check GID for direct routed MADs.
 		     * These might have permissive LIDs.
 		     */
-		    (is_direct(wc->recv_buf.mad->mad_hdr.mgmt_class) ||
+		    (is_direct(mad_hdr->mgmt_class) ||
 		     rcv_has_same_gid(mad_agent_priv, wr, wc)))
 			/* Verify request has not been canceled */
 			return (wr->status == IB_WC_SUCCESS) ? wr : NULL;
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html