opal hotel: only delete events that have not yet fired

The eviction callback, for convenience (and to avoid code
duplication), use to call opal_hotel_checkout().  However,
opal_hotel_checkout() deletes the eviction event -- which is fine to
do when opal_hotel_checkout() is invoked by the application.  But when
it's invoked by the same event that it's deleting, it can cause Bad
Things to happen.

For simplicity, instead of invoking opal_hotel_checkout() from the
eviction callback, just duplicate the checkout logic into the eviction
callback function (and skip the delete-the-evict-event part).

For good measure, put a comment in all three places where the checkout
logic occurs (because it's inlined): don't change this logic without
changing all 3 places.

Finally, also add a line in the docs for opal_hotel_init() warning
users from calling opal_hotel_checkout() from their eviction
callback.

Author: jsquyres

opal/class/opal_hotel.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012      Cisco Systems, Inc. All rights reserved.
+ * Copyright (c) 2012-2016 Cisco Systems, Inc.  All rights reserved.
  * Copyright (c) 2012      Los Alamos National Security, LLC. All rights reserved
  * Copyright (c) 2015      Intel, Inc. All rights reserved
  * $COPYRIGHT$
@@ -24,12 +24,22 @@ static void local_eviction_callback(int fd, short flags, void *arg)
         (opal_hotel_room_eviction_callback_arg_t*) arg;
     void *occupant = eargs->hotel->rooms[eargs->room_num].occupant;
 
-    /* Remove the occupant from the room and invoke the user callback
-       to tell them that they were evicted */
-    opal_hotel_checkout(eargs->hotel, eargs->room_num);
-    eargs->hotel->evict_callback_fn(eargs->hotel,
-                                    eargs->room_num,
-                                    occupant);
+    /* Remove the occurpant from the room.
+
+       Do not change this logic without also changing the same logic
+       in opal_hotel_checkout() and
+       opal_hotel_checkout_and_return_occupant(). */
+    opal_hotel_t *hotel = eargs->hotel;
+    opal_hotel_room_t *room = &(hotel->rooms[eargs->room_num]);
+    room->occupant = NULL;
+    hotel->last_unoccupied_room++;
+    assert(hotel->last_unoccupied_room < hotel->num_rooms);
+    hotel->unoccupied_rooms[hotel->last_unoccupied_room] = eargs->room_num;
+
+    /* Invoke the user callback to tell them that they were evicted */
+    hotel->evict_callback_fn(hotel,
+                             eargs->room_num,
+                             occupant);
 }
 
 

opal/class/opal_hotel.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012-2013 Cisco Systems, Inc. All rights reserved.
+ * Copyright (c) 2012-2016 Cisco Systems, Inc.  All rights reserved.
  * Copyright (c) 2012      Los Alamos National Security, LLC. All rights reserved
  * Copyright (c) 2015      Intel, Inc. All rights reserved.
  * $COPYRIGHT$
@@ -146,6 +146,11 @@ OBJ_CLASS_DECLARATION(opal_hotel_t);
  * will be set - occupants will remain checked into the hotel until
  * explicitly checked out.
  *
+ * Also note: the eviction_callback_fn should absolutely not call any
+ * of the hotel checkout functions.  Specifically: the occupant has
+ * already been ("forcibly") checked out *before* the
+ * eviction_callback_fn is invoked.
+ *
  * @return OPAL_SUCCESS if all initializations were succesful. Otherwise,
  *  the error indicate what went wrong in the function.
  */
@@ -244,6 +249,9 @@ static inline void opal_hotel_checkout(opal_hotel_t *hotel, int room_num)
     /* If there's an occupant in the room, check them out */
     room = &(hotel->rooms[room_num]);
     if (OPAL_LIKELY(NULL != room->occupant)) {
+        /* Do not change this logic without also changing the same
+           logic in opal_hotel_checkout_and_return_occupant() and
+           opal_hotel.c:local_eviction_callback(). */
         room->occupant = NULL;
         if (NULL != hotel->evbase) {
             opal_event_del(&(room->eviction_timer_event));
@@ -280,6 +288,9 @@ static inline void opal_hotel_checkout_and_return_occupant(opal_hotel_t *hotel,
     room = &(hotel->rooms[room_num]);
     if (OPAL_LIKELY(NULL != room->occupant)) {
         opal_output (10, "checking out occupant %p from room num %d", room->occupant, room_num);
+        /* Do not change this logic without also changing the same
+           logic in opal_hotel_checkout() and
+           opal_hotel.c:local_eviction_callback(). */
         *occupant = room->occupant;
         room->occupant = NULL;
         if (NULL != hotel->evbase) {