Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On 12/16/2015 07:16 PM, Jason Gunthorpe wrote: > On Wed, Dec 16, 2015 at 11:26:39AM +0100, Michael Wang wrote: [snip] >> >> I've rechecked the ib_init_ah_from_path() again, and found it >> still set IB_AH_GRH when the GID cache missing, but with: > > How do you mean? > > ah_attr->ah_flags = IB_AH_GRH; > ah_attr->grh.dgid = rec->dgid; > > ret = ib_find_cached_gid(device, >sgid, ndev, _num, > _index); > if (ret) { > if (ndev) > dev_put(ndev); > return ret; > } > > If find_cached_gid fails then ib_init_ah_from_path also fails. > > Is there a case where ib_find_cached_gid can succeed but not return > good data? Just for the GRH header, ib_find_cached_gid() will failed but the flag and dgid will be correct, and others are all 0 including hop limit, but may be just coincidence... As long as hop_limit > 1 means the pkg do have to pass through a router to other subnet, the fix make sense :-) Regards, Michael Wang > > I agree it would read nicer if the ah_flags and gr.dgid was moved > after the ib_find_cached_gid > >> BTW, cma_sidr_rep_handler() also call ib_init_ah_from_path() with out >> a check on return. > > That sounds like a problem. > > Jason > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On 12/16/2015 07:16 PM, Jason Gunthorpe wrote: > On Wed, Dec 16, 2015 at 11:26:39AM +0100, Michael Wang wrote: [snip] >> >> I've rechecked the ib_init_ah_from_path() again, and found it >> still set IB_AH_GRH when the GID cache missing, but with: > > How do you mean? > > ah_attr->ah_flags = IB_AH_GRH; > ah_attr->grh.dgid = rec->dgid; > > ret = ib_find_cached_gid(device, >sgid, ndev, _num, > _index); > if (ret) { > if (ndev) > dev_put(ndev); > return ret; > } > > If find_cached_gid fails then ib_init_ah_from_path also fails. > > Is there a case where ib_find_cached_gid can succeed but not return > good data? Just for the GRH header, ib_find_cached_gid() will failed but the flag and dgid will be correct, and others are all 0 including hop limit, but may be just coincidence... As long as hop_limit > 1 means the pkg do have to pass through a router to other subnet, the fix make sense :-) Regards, Michael Wang > > I agree it would read nicer if the ah_flags and gr.dgid was moved > after the ib_find_cached_gid > >> BTW, cma_sidr_rep_handler() also call ib_init_ah_from_path() with out >> a check on return. > > That sounds like a problem. > > Jason > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On Wed, Dec 16, 2015 at 11:26:39AM +0100, Michael Wang wrote: > > On 12/15/2015 06:30 PM, Jason Gunthorpe wrote: > > On Tue, Dec 15, 2015 at 05:38:34PM +0100, Michael Wang wrote: > >> The hop_limit is only suggest that the package allowed to be > >> routed, not have to, correct? > > > > If the hop limit is >= 2 (?) then the GRH is mandatory. The > > SM will return this information in the PathRecord if it determines a > > GRH is required. The whole stack follows this protocol. > > > > The GRH is optional for in-subnet communications. > > Thanks for the explain :-) > > I've rechecked the ib_init_ah_from_path() again, and found it > still set IB_AH_GRH when the GID cache missing, but with: How do you mean? ah_attr->ah_flags = IB_AH_GRH; ah_attr->grh.dgid = rec->dgid; ret = ib_find_cached_gid(device, >sgid, ndev, _num, _index); if (ret) { if (ndev) dev_put(ndev); return ret; } If find_cached_gid fails then ib_init_ah_from_path also fails. Is there a case where ib_find_cached_gid can succeed but not return good data? I agree it would read nicer if the ah_flags and gr.dgid was moved after the ib_find_cached_gid > BTW, cma_sidr_rep_handler() also call ib_init_ah_from_path() with out > a check on return. That sounds like a problem. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On 12/15/2015 06:30 PM, Jason Gunthorpe wrote: > On Tue, Dec 15, 2015 at 05:38:34PM +0100, Michael Wang wrote: >> The hop_limit is only suggest that the package allowed to be >> routed, not have to, correct? > > If the hop limit is >= 2 (?) then the GRH is mandatory. The > SM will return this information in the PathRecord if it determines a > GRH is required. The whole stack follows this protocol. > > The GRH is optional for in-subnet communications. Thanks for the explain :-) I've rechecked the ib_init_ah_from_path() again, and found it still set IB_AH_GRH when the GID cache missing, but with: grh.sgid_index = 0 grh.flow_label = 0 grh.hop_limit = 0 grh.traffic_class = 0 Not sure if it's just coincidence, hop_limit is 0, so router will discard the pkg and GRH won't be used, the transaction in subnet still works. Could this by designed as an optimization for the case like when SM reassigning the GID? BTW, cma_sidr_rep_handler() also call ib_init_ah_from_path() with out a check on return. Regards, Michael Wang > > Jason > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On Wed, Dec 16, 2015 at 11:26:39AM +0100, Michael Wang wrote: > > On 12/15/2015 06:30 PM, Jason Gunthorpe wrote: > > On Tue, Dec 15, 2015 at 05:38:34PM +0100, Michael Wang wrote: > >> The hop_limit is only suggest that the package allowed to be > >> routed, not have to, correct? > > > > If the hop limit is >= 2 (?) then the GRH is mandatory. The > > SM will return this information in the PathRecord if it determines a > > GRH is required. The whole stack follows this protocol. > > > > The GRH is optional for in-subnet communications. > > Thanks for the explain :-) > > I've rechecked the ib_init_ah_from_path() again, and found it > still set IB_AH_GRH when the GID cache missing, but with: How do you mean? ah_attr->ah_flags = IB_AH_GRH; ah_attr->grh.dgid = rec->dgid; ret = ib_find_cached_gid(device, >sgid, ndev, _num, _index); if (ret) { if (ndev) dev_put(ndev); return ret; } If find_cached_gid fails then ib_init_ah_from_path also fails. Is there a case where ib_find_cached_gid can succeed but not return good data? I agree it would read nicer if the ah_flags and gr.dgid was moved after the ib_find_cached_gid > BTW, cma_sidr_rep_handler() also call ib_init_ah_from_path() with out > a check on return. That sounds like a problem. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On 12/15/2015 06:30 PM, Jason Gunthorpe wrote: > On Tue, Dec 15, 2015 at 05:38:34PM +0100, Michael Wang wrote: >> The hop_limit is only suggest that the package allowed to be >> routed, not have to, correct? > > If the hop limit is >= 2 (?) then the GRH is mandatory. The > SM will return this information in the PathRecord if it determines a > GRH is required. The whole stack follows this protocol. > > The GRH is optional for in-subnet communications. Thanks for the explain :-) I've rechecked the ib_init_ah_from_path() again, and found it still set IB_AH_GRH when the GID cache missing, but with: grh.sgid_index = 0 grh.flow_label = 0 grh.hop_limit = 0 grh.traffic_class = 0 Not sure if it's just coincidence, hop_limit is 0, so router will discard the pkg and GRH won't be used, the transaction in subnet still works. Could this by designed as an optimization for the case like when SM reassigning the GID? BTW, cma_sidr_rep_handler() also call ib_init_ah_from_path() with out a check on return. Regards, Michael Wang > > Jason > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On Tue, Dec 15, 2015 at 10:30:22AM -0700, Jason Gunthorpe wrote: > On Tue, Dec 15, 2015 at 05:38:34PM +0100, Michael Wang wrote: > > The hop_limit is only suggest that the package allowed to be > > routed, not have to, correct? > > If the hop limit is >= 2 (?) then the GRH is mandatory. Yes >= 2. "Setting this value to 0 or 1 will ensure that the packet will not be forwarded beyond the local subnet." -- IBTA 8.3.6 Hop Limit Ira > The > SM will return this information in the PathRecord if it determines a > GRH is required. The whole stack follows this protocol. > > The GRH is optional for in-subnet communications. > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On Tue, Dec 15, 2015 at 05:38:34PM +0100, Michael Wang wrote: > The hop_limit is only suggest that the package allowed to be > routed, not have to, correct? If the hop limit is >= 2 (?) then the GRH is mandatory. The SM will return this information in the PathRecord if it determines a GRH is required. The whole stack follows this protocol. The GRH is optional for in-subnet communications. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On 12/15/2015 04:52 PM, Nicholas Krause wrote: > This adds a needed error path in the function, cm_init_av_by_path > after the call to ib_init_ah_from_path in order to avoid incorrectly > accessing the path pointer of structure type ib_sa_path_rec if this > function call fails to complete its intended work successfully by > returning a error code. > > Signed-off-by: Nicholas Krause > --- > drivers/infiniband/core/cm.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index 0a26dd6..e9b36ea 100644 > --- a/drivers/infiniband/core/cm.c > +++ b/drivers/infiniband/core/cm.c > @@ -383,8 +383,11 @@ static int cm_init_av_by_path(struct ib_sa_path_rec > *path, struct cm_av *av) > return ret; > > av->port = port; > - ib_init_ah_from_path(cm_dev->ib_device, port->port_num, path, > - >ah_attr); > + ret = ib_init_ah_from_path(cm_dev->ib_device, port->port_num, path, > +>ah_attr); ..Just wondering what if the transport don't require GRH? eg inside the same subnet? The hop_limit is only suggest that the package allowed to be routed, not have to, correct? Regards, Michael Wang > + if (ret) > + return ret; > + > av->timeout = path->packet_life_time + 1; > > return 0; > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On 12/15/2015 04:52 PM, Nicholas Krause wrote: > This adds a needed error path in the function, cm_init_av_by_path > after the call to ib_init_ah_from_path in order to avoid incorrectly > accessing the path pointer of structure type ib_sa_path_rec if this > function call fails to complete its intended work successfully by > returning a error code. > > Signed-off-by: Nicholas Krause> --- > drivers/infiniband/core/cm.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index 0a26dd6..e9b36ea 100644 > --- a/drivers/infiniband/core/cm.c > +++ b/drivers/infiniband/core/cm.c > @@ -383,8 +383,11 @@ static int cm_init_av_by_path(struct ib_sa_path_rec > *path, struct cm_av *av) > return ret; > > av->port = port; > - ib_init_ah_from_path(cm_dev->ib_device, port->port_num, path, > - >ah_attr); > + ret = ib_init_ah_from_path(cm_dev->ib_device, port->port_num, path, > +>ah_attr); ..Just wondering what if the transport don't require GRH? eg inside the same subnet? The hop_limit is only suggest that the package allowed to be routed, not have to, correct? Regards, Michael Wang > + if (ret) > + return ret; > + > av->timeout = path->packet_life_time + 1; > > return 0; > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On Tue, Dec 15, 2015 at 05:38:34PM +0100, Michael Wang wrote: > The hop_limit is only suggest that the package allowed to be > routed, not have to, correct? If the hop limit is >= 2 (?) then the GRH is mandatory. The SM will return this information in the PathRecord if it determines a GRH is required. The whole stack follows this protocol. The GRH is optional for in-subnet communications. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND] infiniband:core:Add needed error path in cm_init_av_by_path
On Tue, Dec 15, 2015 at 10:30:22AM -0700, Jason Gunthorpe wrote: > On Tue, Dec 15, 2015 at 05:38:34PM +0100, Michael Wang wrote: > > The hop_limit is only suggest that the package allowed to be > > routed, not have to, correct? > > If the hop limit is >= 2 (?) then the GRH is mandatory. Yes >= 2. "Setting this value to 0 or 1 will ensure that the packet will not be forwarded beyond the local subnet." -- IBTA 8.3.6 Hop Limit Ira > The > SM will return this information in the PathRecord if it determines a > GRH is required. The whole stack follows this protocol. > > The GRH is optional for in-subnet communications. > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/