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

[Krishna Kumar2]

Sorry, the last mail came disfigured :). Here it is in a (hopefully)
more readable form :

Hi Tom & Sean,

> > There may be a race here, but...  Why wouldn't the second call into
> > cm_work_handler simply find the list empty on entry into the call?
> 
> Basically, you've got a free work queue element sitting on the iwcm_wq. 
What
> typically would happen is you'd end up corrupting the list because the
> cm_event_handler would enqueue the element, it would get freed in
> cm_work_handler with put_work, then cm_event_handler would call get_work
> (getting the one just freed that's also sitting on the iwcm_wq list) and 
...
> bad things happen. 

Actually I had anticipated this possible problem when I submitted
the patch, and I have explained (given below) why it is not a problem :

"Doing the redundant queue_work() (if cm_work_handler is
already running 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".

Isn't that correct ? So if cm_work_handler() is already running
and processing the LAST entry (anything but the last entry
will not have an issue as a new queue_work would not be
done by iwcm), it will next find this new entry in it's current
run iteration, and process it. Meanwhile iwcm had done a
redundant "queue_work()" on this queue, which, besides
adding the new entry to the workqueue, also does a wakeup
of "worker_thread" (which is still running the previous iteration
of run_workqueue -> cm_work_handler). When
cm_work_handler finishes removing this new entry, it returns
to worker_thread, which will do a schedule() which gets woken
up again immediately due to the redundant "queue_work" done
earlier, but then it checks whether the list is empty and since it
is empty, it does another "schedule()" call. So that is what I
meant by saying that another call to cm_work_handler() will
NOT result (and where that redundant call would find no entry
to process).

So I feel this patch is correct in it's original form. Comments or
did I misunderstand the kernel code completely ?

Thanks,

- KK

> 
> > As an 
> > alternative, could you defer the list_del_init() call to the end of 
the loop,
> > which would avoid scheduling cm_work_handler while it's running?
> 
> Yeah, that's a good idea.
> 
> > 
> > - Sean
> 
>