[openib-general] RE: [PATCH][kdapl] fix spin_lock_irqsave/spin_unlock_irqrestore i mplementation
Itamar Rabenstein
itamar at mellanox.co.il
Wed May 18 12:44:36 PDT 2005
"evd producer locking" is something that we need in openib kdapl
as it was in Source Forge implementation.
If the evd gets more than one event stream
we need a lock to prevent corruption of the evd,
when 2 streams will try to add event at the same time
The reason for the lock being taken in one function and release in the
othere
can be found directly in the remark before the functions
/*
* Event posting code follows.
*/
/*
* These next two functions (dapl_evd_get_event and dapl_evd_post_event)
* are a pair. They are always called together, from one of the functions
* at the end of this file (dapl_evd_post_*_event).
*
* Note that if producer side locking is enabled, the first one takes the
* EVD lock and the second releases it.
*/
Itamar
> -----Original Message-----
> From: James Lentini [mailto:jlentini at netapp.com]
> Sent: Wednesday, May 18, 2005 7:42 PM
> To: Itamar
> Cc: openib-general
> Subject: Re: [PATCH][kdapl] fix
> spin_lock_irqsave/spin_unlock_irqrestore
> implementation
>
>
>
> Wow, I didn't realize that we did this in the code.
>
> Can we simplify this? I'd rather not have the code taking a lock in
> one function and releasing it in another. This code came from the
> generic source forge implementation. Do we need to do
> "evd producer locking" for OpenIB? If not we can remove these locks.
>
> james
>
> On Wed, 18 May 2005, Itamar wrote:
>
> itamar> when spin_lock_irqsave and spin_unlock_irqrestore are
> not called in the same function
> itamar> need to pass the flags form the save the store
> itamar>
> itamar> Signed-off-by: Itamar Rabenstein <itamar at mellanox.co.il>
> itamar>
> itamar> Index: dapl_evd_util.c
> itamar>
> ===================================================================
> itamar> --- dapl_evd_util.c (revision 2374)
> itamar> +++ dapl_evd_util.c (working copy)
> itamar> @@ -378,20 +378,19 @@
> itamar> * that the lock is held.
> itamar> */
> itamar>
> itamar> -static struct dat_event *dapl_evd_get_event(DAPL_EVD
> * evd_ptr)
> itamar> +static struct dat_event *dapl_evd_get_event(DAPL_EVD
> * evd_ptr, unsigned long *flags)
> itamar> {
> itamar> struct dat_event *event;
> itamar> - unsigned long flags;
> itamar>
> itamar> if (evd_ptr->evd_producer_locking_needed) {
> itamar> - spin_lock_irqsave(&evd_ptr->header.lock, flags);
> itamar> +
> spin_lock_irqsave(&evd_ptr->header.lock, *flags);
> itamar> }
> itamar>
> itamar> event = (struct dat_event *)
> dapl_rbuf_remove(&evd_ptr->free_event_queue);
> itamar>
> itamar> /* Release the lock if it was taken and the
> call failed. */
> itamar> if (!event && evd_ptr->evd_producer_locking_needed) {
> itamar> -
> spin_unlock_irqrestore(&evd_ptr->header.lock, flags);
> itamar> +
> spin_unlock_irqrestore(&evd_ptr->header.lock, *flags);
> itamar> }
> itamar>
> itamar> return event;
> itamar> @@ -406,10 +405,10 @@
> itamar> */
> itamar>
> itamar> static void dapl_evd_post_event(DAPL_EVD *evd_ptr,
> itamar> - const struct dat_event
> *event_ptr)
> itamar> + const struct dat_event
> *event_ptr,
> itamar> + unsigned long flags)
> itamar> {
> itamar> u32 dat_status;
> itamar> - unsigned long flags;
> itamar> DAPL_CNO * cno_to_trigger = NULL;
> itamar>
> itamar> dapl_dbg_log(DAPL_DBG_TYPE_EVD,
> itamar> @@ -459,7 +458,7 @@
> itamar> DAPL_EVD * overflow_evd_ptr)
> itamar> {
> itamar> struct dat_event *overflow_event;
> itamar> -
> itamar> + unsigned long flags;
> itamar> /* The overflow_evd_ptr mght be the same as evd.
> itamar> * In that case we've got a catastrophic overflow.
> itamar> */
> itamar> @@ -469,7 +468,7 @@
> itamar> return;
> itamar> }
> itamar>
> itamar> - overflow_event = dapl_evd_get_event(overflow_evd_ptr);
> itamar> + overflow_event =
> dapl_evd_get_event(overflow_evd_ptr, &flags);
> itamar> if (!overflow_event) {
> itamar> /* this is not good */
> itamar> overflow_evd_ptr->catastrophic_overflow = TRUE;
> itamar> @@ -477,17 +476,18 @@
> itamar> return;
> itamar> }
> itamar>
> dapl_evd_format_overflow_event(overflow_evd_ptr, overflow_event);
> itamar> - dapl_evd_post_event(overflow_evd_ptr, overflow_event);
> itamar> + dapl_evd_post_event(overflow_evd_ptr,
> overflow_event, flags);
> itamar>
> itamar> return;
> itamar> }
> itamar>
> itamar> static struct dat_event
> *dapl_evd_get_and_init_event(DAPL_EVD *evd_ptr,
> itamar> -
enum dat_event_number evno)
> itamar> +
enum dat_event_number evno,
> itamar> +
> unsigned long *flags)
> itamar> {
> itamar> struct dat_event *event_ptr;
> itamar>
> itamar> - event_ptr = dapl_evd_get_event(evd_ptr);
> itamar> + event_ptr = dapl_evd_get_event(evd_ptr, flags);
> itamar> if (!event_ptr)
> itamar>
> dapl_evd_post_overflow_event(evd_ptr->header.owner_ia->
> itamar>
async_error_evd, evd_ptr);
> itamar> @@ -507,7 +507,8 @@
> itamar> DAT_CR_HANDLE cr_handle)
> itamar> {
> itamar> struct dat_event *event_ptr;
> itamar> - event_ptr =
> dapl_evd_get_and_init_event(evd_ptr, event_number);
> itamar> + unsigned long flags;
> itamar> + event_ptr =
> dapl_evd_get_and_init_event(evd_ptr, event_number, &flags);
> itamar> /*
> itamar> * Note event lock may be held on successful return
> itamar> * to be released by dapl_evd_post_event(), if
> provider side locking
> itamar> @@ -525,7 +526,7 @@
> itamar>
> event_ptr->event_data.cr_arrival_event_data.conn_qual = conn_qual;
> itamar>
> event_ptr->event_data.cr_arrival_event_data.cr_handle = cr_handle;
> itamar>
> itamar> - dapl_evd_post_event(evd_ptr, event_ptr);
> itamar> + dapl_evd_post_event(evd_ptr, event_ptr, flags);
> itamar>
> itamar> return DAT_SUCCESS;
> itamar> }
> itamar> @@ -537,7 +538,8 @@
> itamar> void *private_data)
> itamar> {
> itamar> struct dat_event *event_ptr;
> itamar> - event_ptr =
> dapl_evd_get_and_init_event(evd_ptr, event_number);
> itamar> + unsigned long flags;
> itamar> + event_ptr =
> dapl_evd_get_and_init_event(evd_ptr, event_number, &flags);
> itamar> /*
> itamar> * Note event lock may be held on successful return
> itamar> * to be released by dapl_evd_post_event(), if
> provider side locking
> itamar> @@ -554,7 +556,7 @@
> itamar> = private_data_size;
> itamar>
> event_ptr->event_data.connect_event_data.private_data = private_data;
> itamar>
> itamar> - dapl_evd_post_event(evd_ptr, event_ptr);
> itamar> + dapl_evd_post_event(evd_ptr, event_ptr, flags);
> itamar>
> itamar> return DAT_SUCCESS;
> itamar> }
> itamar> @@ -564,7 +566,8 @@
> itamar> DAT_IA_HANDLE ia_handle)
> itamar> {
> itamar> struct dat_event *event_ptr;
> itamar> - event_ptr =
> dapl_evd_get_and_init_event(evd_ptr, event_number);
> itamar> + unsigned long flags;
> itamar> + event_ptr =
> dapl_evd_get_and_init_event(evd_ptr, event_number, &flags);
> itamar> /*
> itamar> * Note event lock may be held on successful return
> itamar> * to be released by dapl_evd_post_event(), if
> provider side locking
> itamar> @@ -579,7 +582,7 @@
> itamar>
> event_ptr->event_data.asynch_error_event_data.dat_handle =
> itamar> (DAT_HANDLE) ia_handle;
> itamar>
> itamar> - dapl_evd_post_event(evd_ptr, event_ptr);
> itamar> + dapl_evd_post_event(evd_ptr, event_ptr, flags);
> itamar>
> itamar> return DAT_SUCCESS;
> itamar> }
> itamar> @@ -589,7 +592,8 @@
> itamar> void *pointer)
> itamar> {
> itamar> struct dat_event *event_ptr;
> itamar> - event_ptr =
> dapl_evd_get_and_init_event(evd_ptr, event_number);
> itamar> + unsigned long flags;
> itamar> + event_ptr =
> dapl_evd_get_and_init_event(evd_ptr, event_number, &flags);
> itamar> /*
> itamar> * Note event lock may be held on successful return
> itamar> * to be released by dapl_evd_post_event(), if
> provider side locking
> itamar> @@ -603,7 +607,7 @@
> itamar>
> itamar>
> event_ptr->event_data.software_event_data.pointer = pointer;
> itamar>
> itamar> - dapl_evd_post_event(evd_ptr, event_ptr);
> itamar> + dapl_evd_post_event(evd_ptr, event_ptr, flags);
> itamar>
> itamar> return DAT_SUCCESS;
> itamar> }
> itamar>
> itamar> --
> itamar> Itamar
> itamar>
>
More information about the general
mailing list