vcl: store libc_epfd in vls instead of vcl

ldp stores the libc epfds it uses in conjunction with vcl epfds in vcl
attributes. Apart from being an ldp, as opposed to vcl, specific
attribute, the epfd is retrieved on session close with vls locks that
could be acquired from multiple threads.

Avoid grabbing locks when retrieving the attribute.

Type: improvement

Signed-off-by: Florin Coras <[email protected]>
Change-Id: I5fc52d72667a7693b1557d209c3890a968422f1e

diff --git a/src/vcl/ldp.c b/src/vcl/ldp.c
index befb308..e8cabd2 100644 (file)
--- a/src/vcl/ldp.c
+++ b/src/vcl/ldp.c
@@ -327,19 +327,17 @@ close (int fd)
   vlsh = ldp_fd_to_vlsh (fd);
   if (vlsh != VLS_INVALID_HANDLE)
     {
-      epfd = vls_attr (vlsh, VPPCOM_ATTR_GET_LIBC_EPFD, 0, 0);
+      epfd = vls_get_libc_epfd (vlsh);
       if (epfd > 0)
        {
          ldp_worker_ctx_t *ldpw = ldp_worker_get_current ();
-         u32 size = sizeof (epfd);
 
          LDBG (0, "fd %d: calling libc_close: epfd %u", fd, epfd);
 
          libc_close (epfd);
          ldpw->mq_epfd_added = 0;
 
-         epfd = 0;
-         (void) vls_attr (vlsh, VPPCOM_ATTR_SET_LIBC_EPFD, &epfd, &size);
+         vls_set_libc_epfd (vlsh, 0);
        }
       else if (PREDICT_FALSE (epfd < 0))
        {
@@ -2460,9 +2458,8 @@ epoll_ctl (int epfd, int op, int fd, struct epoll_event *event)
   else
     {
       int libc_epfd;
-      u32 size = sizeof (epfd);
 
-      libc_epfd = vls_attr (vep_vlsh, VPPCOM_ATTR_GET_LIBC_EPFD, 0, 0);
+      libc_epfd = vls_get_libc_epfd (vep_vlsh);
       if (!libc_epfd)
        {
          LDBG (1, "epfd %d, vep_vlsh %d calling libc_epoll_create1: "
@@ -2475,8 +2472,7 @@ epoll_ctl (int epfd, int op, int fd, struct epoll_event *event)
              goto done;
            }
 
-         rv = vls_attr (vep_vlsh, VPPCOM_ATTR_SET_LIBC_EPFD, &libc_epfd,
-                        &size);
+         rv = vls_set_libc_epfd (vep_vlsh, libc_epfd);
          if (rv < 0)
            {
              errno = -rv;
@@ -2538,7 +2534,7 @@ ldp_epoll_pwait (int epfd, struct epoll_event *events, int maxevents,
   time_to_wait = ((timeout >= 0) ? (double) timeout / 1000 : 0);
   max_time = clib_time_now (&ldpw->clib_time) + time_to_wait;
 
-  libc_epfd = vls_attr (ep_vlsh, VPPCOM_ATTR_GET_LIBC_EPFD, 0, 0);
+  libc_epfd = vls_get_libc_epfd (ep_vlsh);
   if (PREDICT_FALSE (libc_epfd < 0))
     {
       errno = -libc_epfd;
@@ -2616,11 +2612,9 @@ ldp_epoll_pwait_eventfd (int epfd, struct epoll_event *events,
       return -1;
     }
 
-  libc_epfd = vls_attr (ep_vlsh, VPPCOM_ATTR_GET_LIBC_EPFD, 0, 0);
+  libc_epfd = vls_get_libc_epfd (ep_vlsh);
   if (PREDICT_FALSE (!libc_epfd))
     {
-      u32 size = sizeof (epfd);
-
       LDBG (1, "epfd %d, vep_vlsh %d calling libc_epoll_create1: "
            "EPOLL_CLOEXEC", epfd, ep_vlsh);
       libc_epfd = libc_epoll_create1 (EPOLL_CLOEXEC);
@@ -2630,7 +2624,7 @@ ldp_epoll_pwait_eventfd (int epfd, struct epoll_event *events,
          goto done;
        }
 
-      rv = vls_attr (ep_vlsh, VPPCOM_ATTR_SET_LIBC_EPFD, &libc_epfd, &size);
+      rv = vls_set_libc_epfd (ep_vlsh, libc_epfd);
       if (rv < 0)
        {
          errno = -rv;

diff --git a/src/vcl/vcl_locked.c b/src/vcl/vcl_locked.c
index 24cc598..9f3d6b5 100644 (file)
--- a/src/vcl/vcl_locked.c
+++ b/src/vcl/vcl_locked.c
@@ -103,6 +103,7 @@ typedef struct vcl_locked_session_
   u32 shared_data_index;   /**< shared data index if any */
   u32 owner_vcl_wrk_index; /**< vcl wrk of the vls wrk at alloc */
   uword *vcl_wrk_index_to_session_index; /**< map vcl wrk to session */
+  int libc_epfd;                        /**< epoll fd for libc epoll */
 } vcl_locked_session_t;
 
 typedef struct vls_worker_
@@ -2028,6 +2029,52 @@ vls_use_real_epoll (void)
   return vcl_worker_get_current ()->vcl_needs_real_epoll;
 }
 
+int
+vls_set_libc_epfd (vls_handle_t ep_vlsh, int libc_epfd)
+{
+  vcl_locked_session_t *vls;
+
+  vls_mt_detect ();
+  if (!(vls = vls_get_w_dlock (ep_vlsh)))
+    return VPPCOM_EBADFD;
+  if (vls_mt_session_should_migrate (vls))
+    {
+      vls = vls_mt_session_migrate (vls);
+      if (PREDICT_FALSE (!vls))
+       return VPPCOM_EBADFD;
+    }
+  vls->libc_epfd = libc_epfd;
+  vls_unlock (vls);
+  vls_mt_pool_runlock ();
+  return 0;
+}
+
+int
+vls_get_libc_epfd (vls_handle_t ep_vlsh)
+{
+  vcl_locked_session_t *vls;
+  int rv;
+
+  vls_mt_detect ();
+  vls_mt_pool_rlock ();
+
+  vls = vls_get (ep_vlsh);
+  if (!vls)
+    {
+      vls_mt_pool_runlock ();
+      return VPPCOM_EBADFD;
+    }
+
+  /* Avoid locking. In mt scenarios, one thread might be blocking waiting on
+   * the fd while another might be closing it. While closing, this attribute
+   * might be retrieved, so avoid deadlock */
+  rv = vls->libc_epfd;
+
+  vls_mt_pool_runlock ();
+
+  return rv;
+}
+
 void
 vls_register_vcl_worker (void)
 {

diff --git a/src/vcl/vcl_locked.h b/src/vcl/vcl_locked.h
index 3d04a36..bc13140 100644 (file)
--- a/src/vcl/vcl_locked.h
+++ b/src/vcl/vcl_locked.h
@@ -56,6 +56,8 @@ int vls_app_create (char *app_name);
 unsigned char vls_use_eventfd (void);
 unsigned char vls_mt_wrk_supported (void);
 int vls_use_real_epoll (void);
+int vls_set_libc_epfd (vls_handle_t ep_vlsh, int libc_epfd);
+int vls_get_libc_epfd (vls_handle_t ep_vlsh);
 void vls_register_vcl_worker (void);
 
 #endif /* SRC_VCL_VCL_LOCKED_H_ */

diff --git a/src/vcl/vcl_private.h b/src/vcl/vcl_private.h
index 0f1a6d2..7e72b29 100644 (file)
--- a/src/vcl/vcl_private.h
+++ b/src/vcl/vcl_private.h
@@ -163,9 +163,8 @@ typedef struct vcl_session_
   session_handle_t parent_handle;
   u32 listener_index;          /**< index of parent listener (if any) */
   int n_accepted_sessions;     /**< sessions accepted by this listener */
-  vppcom_epoll_t vep;
+  vppcom_epoll_t vep;          /**< epoll context */
   u32 attributes;              /**< see @ref vppcom_session_attr_t */
-  int libc_epfd;
   u32 vrf;
   u16 gso_size;
 

diff --git a/src/vcl/vppcom.c b/src/vcl/vppcom.c
index 4568ac6..d297358 100644 (file)
--- a/src/vcl/vppcom.c
+++ b/src/vcl/vppcom.c
@@ -3893,25 +3893,6 @@ vppcom_session_attr (uint32_t session_handle, uint32_t op,
        rv = VPPCOM_EINVAL;
       break;
 
-    case VPPCOM_ATTR_GET_LIBC_EPFD:
-      rv = session->libc_epfd;
-      VDBG (2, "VPPCOM_ATTR_GET_LIBC_EPFD: libc_epfd %d", rv);
-      break;
-
-    case VPPCOM_ATTR_SET_LIBC_EPFD:
-      if (PREDICT_TRUE (buffer && buflen &&
-                       (*buflen == sizeof (session->libc_epfd))))
-       {
-         session->libc_epfd = *(int *) buffer;
-         *buflen = sizeof (session->libc_epfd);
-
-         VDBG (2, "VPPCOM_ATTR_SET_LIBC_EPFD: libc_epfd %d, buflen %d",
-               session->libc_epfd, *buflen);
-       }
-      else
-       rv = VPPCOM_EINVAL;
-      break;
-
     case VPPCOM_ATTR_GET_PROTOCOL:
       if (buffer && buflen && (*buflen >= sizeof (int)))
        {

diff --git a/src/vcl/vppcom.h b/src/vcl/vppcom.h
index a11db95..ad1c1f4 100644 (file)
--- a/src/vcl/vppcom.h
+++ b/src/vcl/vppcom.h
@@ -148,8 +148,8 @@ typedef enum
   VPPCOM_ATTR_GET_LCL_ADDR,
   VPPCOM_ATTR_SET_LCL_ADDR,
   VPPCOM_ATTR_GET_PEER_ADDR,
-  VPPCOM_ATTR_GET_LIBC_EPFD,
-  VPPCOM_ATTR_SET_LIBC_EPFD,
+  VPPCOM_ATTR_GET_UNUSED,
+  VPPCOM_ATTR_SET_UNUSED,
   VPPCOM_ATTR_GET_PROTOCOL,
   VPPCOM_ATTR_GET_LISTEN,
   VPPCOM_ATTR_GET_ERROR,