[openib-general] [PATCH] RDMA/iwcm: Get rid of extra call to list_empty()

[Tom Tucker]

This patch adds a race that causes an event to be processed twice. 

Each cm_id has a list of pending work stuck on cm_id_priv->work_list.
When new work is created, cm_event_handler needs to know whether it
needs to schedule a call to cm_work_handler by enqueing work on the
iwcm_wq or whether it can just append it to the list of work already
scheduled. 

The race is that the cm_work_handler removes the last element on the
list (leaving it empty) and then releases the lock. The cm_event_handler
checks the list, finds it empty and so schedules another call to
cm_work_handler (which is still running). cm_work_handler processes the
work (previous end of list) and then takes the lock again, checks the
list and voila, finds the work just added by cm_event_handler, so he
processes it. But the work is still queued on the iwcm_wq. So when the
cm_work_handler runs again, it processes that same work element a second
time -- causing all kinds of misadventure.

Tom

On Thu, 2006-11-09 at 09:30 +0530, Krishna Kumar wrote:
> Get rid of extra call to list_empty(), and unnecessary
> variable. Has the side effect of sometimes resulting in
> faster processing of new events (like handling new
> connections, eg when cm_work_handler was processing the
> last entry) added to this list instead of cm_work_handler
> function exiting and re-entering when a new queue_work()
> is done.
> 
> Doing the redundant queue_work() (if cm_work_handler is
> already running and processing the last entry) will not
> result in another call to cm_work_handler (run_workqueue)
> where no entry is found, since cm_work_handler will remove
> all entries from the list, even ones that are added late.
> 
> Signed-off-by: Krishna Kumar <krkumar2 at in.ibm.com>
> ---
> diff -ruNp org/drivers/infiniband/core/iwcm.c new/drivers/infiniband/core/iwcm.c
> --- org/drivers/infiniband/core/iwcm.c	2006-10-09 16:40:04.000000000 +0530
> +++ new/drivers/infiniband/core/iwcm.c	2006-10-09 16:52:03.000000000 +0530
> @@ -834,22 +834,17 @@ static void cm_work_handler(void *arg)
>  	struct iw_cm_event levent;
>  	struct iwcm_id_private *cm_id_priv = work->cm_id;
>  	unsigned long flags;
> -	int empty;
> -	int ret = 0;
>  
>  	spin_lock_irqsave(&cm_id_priv->lock, flags);
> -	empty = list_empty(&cm_id_priv->work_list);
> -	while (!empty) {
> +	while (!list_empty(&cm_id_priv->work_list)) {
>  		work = list_entry(cm_id_priv->work_list.next,
>  				  struct iwcm_work, list);
>  		list_del_init(&work->list);
> -		empty = list_empty(&cm_id_priv->work_list);
>  		levent = work->event;
>  		put_work(work);
>  		spin_unlock_irqrestore(&cm_id_priv->lock, flags);
>  
> -		ret = process_event(cm_id_priv, &levent);
> -		if (ret) {
> +		if (process_event(cm_id_priv, &levent)) {
>  			set_bit(IWCM_F_CALLBACK_DESTROY, &cm_id_priv->flags);
>  			destroy_cm_id(&cm_id_priv->id);
>  		}
> 
> _______________________________________________
> openib-general mailing list
> openib-general at openib.org
> http://openib.org/mailman/listinfo/openib-general
> 
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
>