Re: [PATCH] opensm/osm_trap_rcv.c: In trap_rcv_process_request, no need to sweep on trap 145 and certain trap 144s

[Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>]

On 11:23 Fri 30 Oct     , Hal Rosenstock wrote:
> >
> > In general: function trap_rcv_process_request() already has 400 lines of
> > code and adding there more lines without simplifying flows makes me
> > nervous.
> 
> Are you going to block this change until such restructure ?

I would prefer to improve it at least in a part where patch adds even
more flows. Also there are many issues yet without restructuring.

> 
> >> -     /* Check for node description update. IB Spec v1.2.1 pg 823 */
> >> -     if (ib_notice_is_generic(p_ntci) &&
> >> -         cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 144 &&
> >> -         p_ntci->data_details.ntc_144.local_changes & TRAP_144_MASK_OTHER_LOCAL_CHANGES &&
> >> -         p_ntci->data_details.ntc_144.change_flgs & TRAP_144_MASK_NODE_DESCRIPTION_CHANGE) {
> >> -             OSM_LOG(sm->p_log, OSM_LOG_INFO, "Trap 144 Node description update\n");
> >> -
> >> -             if (p_physp) {
> >> -                     CL_PLOCK_ACQUIRE(sm->p_lock);
> >> -                     osm_req_get_node_desc(sm, p_physp);
> >> -                     CL_PLOCK_RELEASE(sm->p_lock);
> >> -             } else
> >> -                     OSM_LOG(sm->p_log, OSM_LOG_ERROR,
> >> -                             "ERR 3812: No physical port found for "
> >> -                             "trap 144: \"node description update\"\n");
> >> -     }
> >> +     if (ib_notice_is_generic(p_ntci)) {
> >> +             /* Check for node description update. IB Spec v1.2.1 pg 823 */
> >> +             if (cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 144) {
> >> +                     /* update port's capability mask (in PortInfo) */
> >> +                     p_physp->port_info.capability_mask = p_ntci->data_details.ntc_144.new_cap_mask;
> >
> > Hmm, could this introduce some issues?
> 
> Like what ?

Like ignoring isSM bit and maybe some other "problematic" bits which
required SM DB update.

> 
> >> +                     if (p_ntci->data_details.ntc_144.local_changes & TRAP_144_MASK_OTHER_LOCAL_CHANGES) {
> >> +                             if (p_ntci->data_details.ntc_144.change_flgs & TRAP_144_MASK_NODE_DESCRIPTION_CHANGE) {
> >
> >        if ((p_ntci->data_details.ntc_144.local_changes &
> >             TRAP_144_MASK_OTHER_LOCAL_CHANGES) &&
> >            (p_ntci->data_details.ntc_144.change_flgs &
> >             TRAP_144_MASK_NODE_DESCRIPTION_CHANGE)) {
> >
> >> +                                     OSM_LOG(sm->p_log, OSM_LOG_INFO,
> >> +                                             "Trap 144 Node description update\n");
> >> +
> >> +                                     if (p_physp) {
> >> +                                             CL_PLOCK_ACQUIRE(sm->p_lock);
> >> +                                             osm_req_get_node_desc(sm, p_physp);
> >> +                                             CL_PLOCK_RELEASE(sm->p_lock);
> >> +                                     } else
> >> +                                             OSM_LOG(sm->p_log, OSM_LOG_ERROR,
> >> +                                                     "ERR 3812: No physical port found for "
> >> +                                                     "trap 144: \"node description update\"\n");
> >> +                             }
> >> +                     }
> >> +                     if (p_ntci->data_details.ntc_144.change_flgs & TRAP_144_MASK_LINK_WIDTH_ENABLE_CHANGE ||
> >> +                         p_ntci->data_details.ntc_144.change_flgs & TRAP_144_MASK_LINK_SPEED_ENABLE_CHANGE ||
> >> +                         p_ntci->data_details.ntc_144.change_flgs & TRAP_144_MASK_SM_PRIORITY_CHANGE)
> >
> >        if ((p_ntci->data_details.ntc_144.change_flgs &
> >             (TRAP_144_MASK_LINK_WIDTH_ENABLE_CHANGE |
> >              TRAP_144_MASK_LINK_SPEED_ENABLE_CHANGE |
> >              TRAP_144_MASK_SM_PRIORITY_CHANGE)))
> >
> >
> >> +                             is_trap144_sweep = TRUE;
> >> +             }
> >>
> >> -     /* do a sweep if we received a trap */
> >> -     if (sm->p_subn->opt.sweep_on_trap) {
> >> -             /* if this is trap number 128 or run_heavy_sweep is TRUE -
> >> -                update the force_heavy_sweep flag of the subnet.
> >> -                Sweep also on traps 144/145 - these traps signal a change of
> >> -                certain port capabilities/system image guid.
> >> -                TODO: In the future this can be changed to just getting
> >> -                PortInfo on this port instead of sweeping the entire subnet. */
> >> -             if (ib_notice_is_generic(p_ntci) &&
> >> -                 (cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 128 ||
> >> -                  cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 144 ||
> >> -                  cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 145 ||
> >> -                  run_heavy_sweep)) {
> >> -                     OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
> >> -                             "Forcing heavy sweep. Received trap:%u\n",
> >> -                             cl_ntoh16(p_ntci->g_or_v.generic.trap_num));
> >> +             if (cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 145) {
> >> +                     /* update system image guid (in NodeInfo) */
> >> +                     p_node = osm_physp_get_node_ptr(p_physp);
> >> +                     if (p_node)
> >
> > When 'p_node' could be NULL?
> 
> It's just a possible return from osm_physp_get_node_ptr and wanted to
> be sure this was not the case before setting node_info. Should this be
> eliminated ?

If p_physp->p_node cannot be NULL legally, then the check should be
eliminated in order to not hide an errors and to not eat cpu cycles.

> 
> >> +                             p_node->node_info.node_guid = p_ntci->data_details.ntc_145.new_sys_guid;
> >
> > Didn't you mean 'node_info.sys_guid' and 'node_info.node_guid'?
> 
> Just sys_guid.

Sorry typo, I tried to ask: shouldn't be:

	p_node->node_info.sys_guid = p_ntci->data_details.ntc_145.new_sys_guid;

, instead?

> 
> >> +             }
> >> +
> >> +             /* do a sweep if we received a trap */
> >> +             if (sm->p_subn->opt.sweep_on_trap) {
> >> +                     /* if this is trap number 128 or run_heavy_sweep is
> >> +                        TRUE - update the force_heavy_sweep flag of the
> >> +                        subnet. Also, sweep on certain types of trap 144.
> >> +                        TODO: In the future this can be changed to just
> >> +                        getting PortInfo on this port instead of sweeping
> >> +                        the entire subnet. */
> >> +                     if (cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 128 ||
> >> +                         is_trap144_sweep || run_heavy_sweep) {
> >
> > It looks that prevents sweeping on PortInfo:CapMask change which
> > is effectively breaks at least SM handover - new SM will be undetected.
> 
> Yes, CapMask change should be included along with the other local
> changes to cause sweep.

So this should change the patch. 

Sasha
--
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