On Sun, Nov 1, 2009 at 11:49 AM, Sasha Khapyorsky <[email protected]> wrote:
> 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.
The latter is a horse of a different color IMO.
>>
>> >> - /* 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.
You had already pointed that issue out elsewhere in your email.
>>
>> >> + 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?
Yes, that's what I meant when I wrote "just sys_guid".
>>
>> >> + }
>> >> +
>> >> + /* 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.
Yes.
-- Hal
> Sasha
>
