[ofa-general] Re: [PATCH v3] opensm: osm_state_mgr.c - purge idle queue if heavy sweep requested

[Yevgeny Kliteynik]

Sasha Khapyorsky wrote:
> Hi Yevgeny,
> 
> On 11:30 Tue 25 Dec     , Yevgeny Kliteynik wrote:
>> If a heavy sweep requested during idle queue processing, OSM continues
>> to process it till the end and only then notices the heavy sweep request.
>> In some cases this might leave a topology change unhandled for several
>> minutes. Instead, OSM should purge the idle queue, which will cause it
>> to start the heavy sweep immediately.
>> When the heavy sweep will be completed, OSM will recalculate multicast
>> routing.
>>
>> Signed-off-by: Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il>
>> ---
>>  opensm/opensm/osm_state_mgr.c |   30 +++++++++++++++++++++++++++++-
>>  1 files changed, 29 insertions(+), 1 deletions(-)
>>
>> diff --git a/opensm/opensm/osm_state_mgr.c b/opensm/opensm/osm_state_mgr.c
>> index 5c39f11..2172ea0 100644
>> --- a/opensm/opensm/osm_state_mgr.c
>> +++ b/opensm/opensm/osm_state_mgr.c
>> @@ -1062,6 +1062,28 @@ static osm_signal_t __process_idle_time_queue_start(IN osm_state_mgr_t *
>>  }
>>
>>  /**********************************************************************
>> + **********************************************************************/
>> +static void __process_idle_time_queue_purge(IN osm_state_mgr_t * const p_mgr)
>> +{
>> +	cl_qlist_t *p_list = &p_mgr->idle_time_list;
>> +	cl_list_item_t *p_list_item;
>> +	osm_idle_item_t *p_process_item;
>> +
>> +	OSM_LOG_ENTER(p_mgr->p_log, __process_idle_time_queue_done);
>> +
>> +	cl_spinlock_acquire(&p_mgr->idle_lock);
>> +	p_list_item = cl_qlist_remove_head(p_list);
>> +	while (p_list_item != cl_qlist_end(p_list)) {
>> +		p_process_item = (osm_idle_item_t *) p_list_item;
>> +		free(p_process_item);
>> +	}
>> +	cl_spinlock_release(&p_mgr->idle_lock);
>> +
>> +	OSM_LOG_EXIT(p_mgr->p_log);
>> +	return;
>> +}
>> +
>> +/**********************************************************************
>>   * Go over all the remote SMs (as updated in the sm_guid_tbl).
>>   * Find if there is a remote sm that is a master SM.
>>   * If there is a remote master SM - return a pointer to it,
>> @@ -1607,11 +1629,17 @@ void osm_state_mgr_process(IN osm_state_mgr_t * const p_mgr,
>>  				/* CALL the done function */
>>  				__process_idle_time_queue_done(p_mgr);
>>
>> +				if (p_mgr->p_subn->force_immediate_heavy_sweep)
>> +					/*
>> +					 * Immediate heavy sweep is requested, so it's
>> +					 * more important than processing idle queue, and
>> +					 * new heavy sweep makes rest of idle queue obsolete.
>> +					 */
>> +					__process_idle_time_queue_purge(p_mgr);
> 
> I think here
> 
> 	signal = OSM_SIGNAL_NONE;
> 	p_mgr->state = OSM_SM_STATE_IDLE;
> 
> should be added in order to not spend one more idle queue cycle. Right?

Basically - yes. That is what I did in the v2 of the patch.
But here I thought to leave the flow as much intact as I could.

I don't have a real example to show that changing the state
and signal would be bad. I's just a general feeling that doing
it would be kind of a hack, while w/o these two additional lines
state manager continues its usual flow after purging the queue.

The price of one additional iteration doesn't look as something
we should consider, especially giving the fact that this whole
flow is relevant only in extreme cases.

-- Yevgeny


> Sasha
> 
>>  				/*
>>  				 * Set the signal to OSM_SIGNAL_IDLE_TIME_PROCESS
>>  				 * so that the next element in the queue gets processed
>>  				 */
>> -
>>  				signal = OSM_SIGNAL_IDLE_TIME_PROCESS;
>>  				p_mgr->state = OSM_SM_STATE_PROCESS_REQUEST;
>>  				break;
>> -- 
>> 1.5.1.4
>>
>>
>