* Re: [PATCH] opensm/osm_trap_rcv.c: In trap_rcv_process_request, no need to sweep on trap 145 and certain trap 144s
[not found] ` <20090805222737.GA8523-Wuw85uim5zDR7s880joybQ@public.gmane.org>
@ 2009-10-30 1:57 ` Sasha Khapyorsky
2009-10-30 15:23 ` Hal Rosenstock
0 siblings, 1 reply; 4+ messages in thread
From: Sasha Khapyorsky @ 2009-10-30 1:57 UTC (permalink / raw)
To: Hal Rosenstock; +Cc: linux-rdma
Hi Hal,
On 18:27 Wed 05 Aug , Hal Rosenstock wrote:
>
> NodeDescription changed trap only needs to query the new NodeDescription
> and not cause sweep
>
> Similarly for SystemImageGUID changed (trap 145)
>
> LinkWidth/SpeedEnabled changed traps (at least right now) and
> SM priority changed traps do need to sweep.
>
> In the future, LinkWidth/SpeedEnabled changed trap handling
> can query PortInfo (may also need to bounce port too).
>
> Also, as noted in related email thread, it's unclear what
> sweeping on non generic traps accomplishes but this behavior
> is preserved.
>
> Signed-off-by: Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> diff --git a/opensm/opensm/osm_trap_rcv.c b/opensm/opensm/osm_trap_rcv.c
> index d2e4202..e5bd529 100644
> --- a/opensm/opensm/osm_trap_rcv.c
> +++ b/opensm/opensm/osm_trap_rcv.c
> @@ -291,8 +291,9 @@ trap_rcv_process_request(IN osm_sm_t * sm,
> osm_physp_t *p_physp;
> cl_ptr_vector_t *p_tbl;
> osm_port_t *p_port;
> + osm_node_t *p_node;
> ib_net16_t source_lid = 0;
> - boolean_t is_gsi = TRUE;
> + boolean_t is_gsi = TRUE, is_trap144_sweep = FALSE;
What about reusing already existing 'run_heavy_sweep' variable and not
introduce new one?
> uint8_t port_num = 0;
> boolean_t physp_change_trap = FALSE;
> uint64_t event_wheel_timeout = OSM_DEFAULT_TRAP_SUPRESSION_TIMEOUT;
> @@ -547,44 +548,59 @@ trap_rcv_process_request(IN osm_sm_t * sm,
> }
> }
In general: function trap_rcv_process_request() already has 400 lines of
code and adding there more lines without simplifying flows makes me
nervous.
>
> - /* 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?
> + 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?
> + 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'?
> + }
> +
> + /* 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.
> + 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));
>
> - sm->p_subn->force_heavy_sweep = TRUE;
> + sm->p_subn->force_heavy_sweep = TRUE;
> + }
> + osm_sm_signal(sm, OSM_SIGNAL_SWEEP);
> }
> + } else if (sm->p_subn->opt.sweep_on_trap)
> osm_sm_signal(sm, OSM_SIGNAL_SWEEP);
> - }
>
> /* If we reached here due to trap 129/130/131 - do not need to do
> the notice report. Just goto exit. We know this is the case
>
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] opensm/osm_trap_rcv.c: In trap_rcv_process_request, no need to sweep on trap 145 and certain trap 144s
2009-10-30 1:57 ` [PATCH] opensm/osm_trap_rcv.c: In trap_rcv_process_request, no need to sweep on trap 145 and certain trap 144s Sasha Khapyorsky
@ 2009-10-30 15:23 ` Hal Rosenstock
[not found] ` <f0e08f230910300823j56edb800nf88efca23165bf5-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Hal Rosenstock @ 2009-10-30 15:23 UTC (permalink / raw)
To: Sasha Khapyorsky; +Cc: linux-rdma
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 9862 bytes --]
Hi Sasha,
On Thu, Oct 29, 2009 at 9:57 PM, Sasha Khapyorsky <sashak@voltaire.com> wrote:
> Hi Hal,
>
> On 18:27 Wed 05 Aug   , Hal Rosenstock wrote:
>>
>> NodeDescription changed trap only needs to query the new NodeDescription
>> and not cause sweep
>>
>> Similarly for SystemImageGUID changed (trap 145)
>>
>> LinkWidth/SpeedEnabled changed traps (at least right now) and
>> SM priority changed traps do need to sweep.
>>
>> In the future, LinkWidth/SpeedEnabled changed trap handling
>> can query PortInfo (may also need to bounce port too).
>>
>> Also, as noted in related email thread, it's unclear what
>> sweeping on non generic traps accomplishes but this behavior
>> is preserved.
>>
>> Signed-off-by: Hal Rosenstock <hal.rosenstock@gmail.com>
>> ---
>> diff --git a/opensm/opensm/osm_trap_rcv.c b/opensm/opensm/osm_trap_rcv.c
>> index d2e4202..e5bd529 100644
>> --- a/opensm/opensm/osm_trap_rcv.c
>> +++ b/opensm/opensm/osm_trap_rcv.c
>> @@ -291,8 +291,9 @@ trap_rcv_process_request(IN osm_sm_t * sm,
>> Â Â Â osm_physp_t *p_physp;
>> Â Â Â cl_ptr_vector_t *p_tbl;
>> Â Â Â osm_port_t *p_port;
>> + Â Â osm_node_t *p_node;
>> Â Â Â ib_net16_t source_lid = 0;
>> - Â Â boolean_t is_gsi = TRUE;
>> + Â Â boolean_t is_gsi = TRUE, is_trap144_sweep = FALSE;
>
> What about reusing already existing 'run_heavy_sweep' variable and not
> introduce new one?
Sure.
>> Â Â Â uint8_t port_num = 0;
>> Â Â Â boolean_t physp_change_trap = FALSE;
>> Â Â Â uint64_t event_wheel_timeout = OSM_DEFAULT_TRAP_SUPRESSION_TIMEOUT;
>> @@ -547,44 +548,59 @@ trap_rcv_process_request(IN osm_sm_t * sm,
>> Â Â Â Â Â Â Â }
>> Â Â Â }
>
> 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 ?
>> - Â Â /* 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 ?
>> + Â Â Â Â Â Â Â Â Â Â 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 ?
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â 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.
>> + Â Â Â Â Â Â }
>> +
>> + Â Â Â Â Â Â /* 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.
-- Hal
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â 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));
>>
>> - Â Â Â Â Â Â Â Â Â Â sm->p_subn->force_heavy_sweep = TRUE;
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â sm->p_subn->force_heavy_sweep = TRUE;
>> + Â Â Â Â Â Â Â Â Â Â }
>> + Â Â Â Â Â Â Â Â Â Â osm_sm_signal(sm, OSM_SIGNAL_SWEEP);
>> Â Â Â Â Â Â Â }
>> + Â Â } else if (sm->p_subn->opt.sweep_on_trap)
>> Â Â Â Â Â Â Â osm_sm_signal(sm, OSM_SIGNAL_SWEEP);
>> - Â Â }
>>
>> Â Â Â /* If we reached here due to trap 129/130/131 - do not need to do
>> Â Â Â Â Â the notice report. Just goto exit. We know this is the case
>>
> --
> 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
>
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±ÙšŠ{ayº\x1dʇڙë,j\a¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] opensm/osm_trap_rcv.c: In trap_rcv_process_request, no need to sweep on trap 145 and certain trap 144s
[not found] ` <f0e08f230910300823j56edb800nf88efca23165bf5-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2009-11-01 15:49 ` Sasha Khapyorsky
2009-11-02 15:27 ` Hal Rosenstock
0 siblings, 1 reply; 4+ messages in thread
From: Sasha Khapyorsky @ 2009-11-01 15:49 UTC (permalink / raw)
To: Hal Rosenstock; +Cc: linux-rdma
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] opensm/osm_trap_rcv.c: In trap_rcv_process_request, no need to sweep on trap 145 and certain trap 144s
2009-11-01 15:49 ` Sasha Khapyorsky
@ 2009-11-02 15:27 ` Hal Rosenstock
0 siblings, 0 replies; 4+ messages in thread
From: Hal Rosenstock @ 2009-11-02 15:27 UTC (permalink / raw)
To: Sasha Khapyorsky; +Cc: linux-rdma
On Sun, Nov 1, 2009 at 11:49 AM, Sasha Khapyorsky <sashak@voltaire.com> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-11-02 15:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20090805222737.GA8523@comcast.net>
[not found] ` <20090805222737.GA8523-Wuw85uim5zDR7s880joybQ@public.gmane.org>
2009-10-30 1:57 ` [PATCH] opensm/osm_trap_rcv.c: In trap_rcv_process_request, no need to sweep on trap 145 and certain trap 144s Sasha Khapyorsky
2009-10-30 15:23 ` Hal Rosenstock
[not found] ` <f0e08f230910300823j56edb800nf88efca23165bf5-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-11-01 15:49 ` Sasha Khapyorsky
2009-11-02 15:27 ` Hal Rosenstock
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox