Re: [PATCH] opensm/osm_trap_rcv.c: More minor reorg of trap_rcv_process_request

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

On 06:50 Mon 02 Nov     , Hal Rosenstock wrote:
> 
> Move some code out into separate handle_babbling_port routine
> for readability/maintainability 
> 
> 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 5790461..1621fbc 100644
> --- a/opensm/opensm/osm_trap_rcv.c
> +++ b/opensm/opensm/osm_trap_rcv.c
> @@ -312,6 +312,61 @@ static void log_trap_info(osm_log_t *p_log, ib_mad_notice_attr_t *p_ntci,
>  			cl_ntoh16(source_lid), cl_ntoh64(trans_id));
>  }
>  
> +static int handle_babbling_port(osm_sm_t *sm, ib_mad_notice_attr_t *p_ntci,
> +				uint32_t num_received,
> +				boolean_t physp_change_trap,
> +				osm_physp_t **pp_physp,
> +				boolean_t *run_heavy_sweep,
> +				uint64_t *event_wheel_timeout)
> +{
> +	if (print_num_received(num_received))
> +		OSM_LOG(sm->p_log, OSM_LOG_ERROR, "ERR 3804: "
> +			"Received trap %u times consecutively\n",
> +			num_received);
> +	/* If the trap provides info about a bad port, mark it as unhealthy. */
> +	if (physp_change_trap == TRUE) {
> +		/* get the port */
> +		*pp_physp = get_physp_by_lid_and_num(sm,
> +						     cl_ntoh16(p_ntci->data_details.ntc_129_131.lid),
> +						     p_ntci->data_details.ntc_129_131.port_num);
> +
> +		if (!*pp_physp)
> +			OSM_LOG(sm->p_log, OSM_LOG_ERROR, "ERR 3805: "
> +				"Failed to find physical port by lid:%u num:%u\n",
> +				cl_ntoh16(p_ntci->data_details.ntc_129_131.lid),
> +				p_ntci->data_details.ntc_129_131.port_num);
> +		else {
> +			/* When babbling port policy option is enabled and
> +			   Threshold for disabling a "babbling" port is exceeded */
> +			if (sm->p_subn->opt.babbling_port_policy &&
> +			    num_received >= 250 &&
> +			    disable_port(sm, *pp_physp) == 0)
> +				return 1;
> +
> +			OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
> +				"Marking unhealthy physical port by lid:%u num:%u\n",
> +				cl_ntoh16(p_ntci->data_details.ntc_129_131.lid),
> +				p_ntci->data_details.ntc_129_131.port_num);
> +			/* check if the current state of the p_physp is healthy. If
> +			   it is - then this is a first change of state. Run a heavy sweep.
> +			   if it is not - no need to mark it again - just restart the timer. */
> +			if (osm_physp_is_healthy(*pp_physp)) {
> +				osm_physp_set_health(*pp_physp, FALSE);
> +				/* Make sure we sweep again - force a heavy sweep. */
> +				/* The sweep should be done only after the re-registration, or
> +				   else we'll be losing track of the timer. */
> +				*run_heavy_sweep = TRUE;
> +			}
> +			/* If we are marking the port as unhealthy - we want to
> +			   keep this for a longer period of time than the
> +			   OSM_DEFAULT_TRAP_SUPRESSION_TIMEOUT. Use the
> +			   OSM_DEFAULT_UNHEALTHY_TIMEOUT */
> +			*event_wheel_timeout = OSM_DEFAULT_UNHEALTHY_TIMEOUT;
> +	}
> +	return 0;
> +}
> +

It looks that you are moving the code as it is to a separate function
where many flow switchers (run_heavy_sweep, event_wheel_timeout,
pp_physp) are magically modified.

What about something like below instead?

Sasha


>From 5ed9114a32f9536123af4bd135c7cc603d340f92 Mon Sep 17 00:00:00 2001
From: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
Date: Tue, 3 Nov 2009 03:46:54 +0200
Subject: [PATCH] opensm/osm_trap_rcv.c: some consolidation

Consolidate noisy port handling code in shutup_noisy_port() function.
As before it will try to disable port or to mark it unhealthy.

Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
---
 opensm/opensm/osm_trap_rcv.c |  113 ++++++++++++++++++++----------------------
 1 files changed, 53 insertions(+), 60 deletions(-)

diff --git a/opensm/opensm/osm_trap_rcv.c b/opensm/opensm/osm_trap_rcv.c
index 0ee4e77..dae892e 100644
--- a/opensm/opensm/osm_trap_rcv.c
+++ b/opensm/opensm/osm_trap_rcv.c
@@ -226,7 +226,6 @@ static int disable_port(osm_sm_t *sm, osm_physp_t *p)
 	uint8_t payload[IB_SMP_DATA_SIZE];
 	osm_madw_context_t context;
 	ib_port_info_t *pi = (ib_port_info_t *)payload;
-	int ret;
 
 	/* select the nearest port to master opensm */
 	if (p->p_remote_physp &&
@@ -236,10 +235,6 @@ static int disable_port(osm_sm_t *sm, osm_physp_t *p)
 	/* If trap 131, might want to disable peer port if available */
 	/* but peer port has been observed not to respond to SM requests */
 
-	OSM_LOG(sm->p_log, OSM_LOG_ERROR, "ERR 3810: "
-		"Disabling physical port 0x%016" PRIx64 " num:%u\n",
-		cl_ntoh64(osm_physp_get_port_guid(p)), p->port_num);
-
 	memcpy(payload, &p->port_info, sizeof(ib_port_info_t));
 
 	/* Set port to disabled/down */
@@ -253,15 +248,10 @@ static int disable_port(osm_sm_t *sm, osm_physp_t *p)
 	context.pi_context.light_sweep = FALSE;
 	context.pi_context.active_transition = FALSE;
 
-	ret = osm_req_set(sm, osm_physp_get_dr_path_ptr(p),
-			  payload, sizeof(payload), IB_MAD_ATTR_PORT_INFO,
-			  cl_hton32(osm_physp_get_port_num(p)),
-			  CL_DISP_MSGID_NONE, &context);
-	if (ret)
-		OSM_LOG(sm->p_log, OSM_LOG_ERROR, "ERR 3811: "
-			"Request to set PortInfo failed\n");
-
-	return ret;
+	return osm_req_set(sm, osm_physp_get_dr_path_ptr(p),
+			   payload, sizeof(payload), IB_MAD_ATTR_PORT_INFO,
+			   cl_hton32(osm_physp_get_port_num(p)),
+			   CL_DISP_MSGID_NONE, &context);
 }
 
 static void log_trap_info(osm_log_t *p_log, ib_mad_notice_attr_t *p_ntci,
@@ -301,6 +291,42 @@ static void log_trap_info(osm_log_t *p_log, ib_mad_notice_attr_t *p_ntci,
 			cl_ntoh16(source_lid), cl_ntoh64(trans_id));
 }
 
+static int shutup_noisy_port(osm_sm_t *sm, uint16_t lid, uint8_t port,
+			     unsigned num)
+{
+	osm_physp_t *p = get_physp_by_lid_and_num(sm, lid, port);
+	if (!p) {
+		OSM_LOG(sm->p_log, OSM_LOG_ERROR, "ERR 3805: "
+			"Failed to find physical port by lid:%u num:%u\n",
+			lid, port);
+		return -1;
+	}
+
+	/* When babbling port policy option is enabled and
+	   Threshold for disabling a "babbling" port is exceeded */
+	if (sm->p_subn->opt.babbling_port_policy && num >= 250) {
+		OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
+			"Disabling noisy physical port 0x%016" PRIx64
+			": lid %u, num %u\n",
+			cl_ntoh64(osm_physp_get_port_guid(p)), lid, port);
+		if (disable_port(sm, p))
+			OSM_LOG(sm->p_log, OSM_LOG_ERROR, "ERR 3811: "
+				"Failed to disable.\n");
+		else
+			return 1;
+	}
+
+	/* check if the current state of the p_physp is healthy. If
+	   it is - then this is a first change of state. Run a heavy sweep. */
+	if (osm_physp_is_healthy(p)) {
+		OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
+			"Marking unhealthy physical port by lid:%u num:%u\n",
+			lid, port);
+		osm_physp_set_health(p, FALSE);
+		return 2;
+	}
+	return 0;
+}
 /**********************************************************************
  **********************************************************************/
 static void trap_rcv_process_request(IN osm_sm_t * sm,
@@ -438,7 +464,7 @@ static void trap_rcv_process_request(IN osm_sm_t * sm,
 		/* Now we know how many times it provided this trap */
 		if (num_received > 10) {
 			if (print_num_received(num_received))
-				OSM_LOG(sm->p_log, OSM_LOG_ERROR, "ERR 3804: "
+				OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
 					"Received trap %u times consecutively\n",
 					num_received);
 			/*
@@ -446,49 +472,17 @@ static void trap_rcv_process_request(IN osm_sm_t * sm,
 			 * we mark it as unhealthy.
 			 */
 			if (physp_change_trap == TRUE) {
-				/* get the port */
-				p_physp = get_physp_by_lid_and_num(sm,
-								   cl_ntoh16
-								   (source_lid),
-								   port_num);
-
-				if (!p_physp)
-					OSM_LOG(sm->p_log, OSM_LOG_ERROR,
-						"ERR 3805: "
-						"Failed to find physical port by lid:%u num:%u\n",
-						cl_ntoh16(source_lid),
-						port_num);
-				else {
-					/* When babbling port policy option is enabled and
-					   Threshold for disabling a "babbling" port is exceeded */
-					if (sm->p_subn->opt.
-					    babbling_port_policy
-					    && num_received >= 250
-					    && disable_port(sm, p_physp) == 0)
-						goto Exit;
-
-					OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
-						"Marking unhealthy physical port by lid:%u num:%u\n",
-						cl_ntoh16(source_lid),
-						port_num);
-					/* check if the current state of the p_physp is healthy. If
-					   it is - then this is a first change of state. Run a heavy sweep.
-					   if it is not - no need to mark it again - just restart the timer. */
-					if (osm_physp_is_healthy(p_physp)) {
-						osm_physp_set_health(p_physp,
-								     FALSE);
-						/* Make sure we sweep again - force a heavy sweep. */
-						/* The sweep should be done only after the re-registration, or
-						   else we'll be losing track of the timer. */
-						run_heavy_sweep = TRUE;
-					}
-					/* If we are marking the port as unhealthy - we want to
-					   keep this for a longer period of time than the
-					   OSM_DEFAULT_TRAP_SUPRESSION_TIMEOUT. Use the
-					   OSM_DEFAULT_UNHEALTHY_TIMEOUT */
-					event_wheel_timeout =
-					    OSM_DEFAULT_UNHEALTHY_TIMEOUT;
-				}
+				int ret = shutup_noisy_port(sm,
+							    cl_ntoh16(source_lid),
+							    port_num,
+							    num_received);
+				if (ret == 1) /* port disabled */
+					goto Exit;
+				else if (ret == 2) /* unhealthy - run sweep */
+					run_heavy_sweep = TRUE;
+				/* in any case increase timeout interval */
+				event_wheel_timeout =
+				    OSM_DEFAULT_UNHEALTHY_TIMEOUT;
 			}
 		}
 
@@ -508,8 +502,7 @@ static void trap_rcv_process_request(IN osm_sm_t * sm,
 		if (num_received > 10 && run_heavy_sweep == FALSE) {
 			if (print_num_received(num_received))
 				OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
-					"Continuously received this trap %u times. Ignoring\n",
-					num_received);
+					"Ignoring noisy traps.\n");
 			goto Exit;
 		}
 	}
-- 
1.6.5.1

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