git.99rst.org Git - openwrt-packages.git/blob

   1 commit 0ff395c154ad827c0c30eefc9371ba7f7c171027
   2 Author: Willy Tarreau <w@1wt.eu>
   3 Date:   Tue Jul 30 11:59:34 2019 +0200
   4
   5     BUG/MAJOR: queue/threads: avoid an AB/BA locking issue in process_srv_queue()
   6
   7     A problem involving server slowstart was reported by @max2k1 in issue #197.
   8     The problem is that pendconn_grab_from_px() takes the proxy lock while
   9     already under the server's lock while process_srv_queue() first takes the
  10     proxy's lock then the server's lock.
  11
  12     While the latter seems more natural, it is fundamentally incompatible with
  13     mayn other operations performed on servers, namely state change propagation,
  14     where the proxy is only known after the server and cannot be locked around
  15     the servers. Howwever reversing the lock in process_srv_queue() is trivial
  16     and only the few functions related to dynamic cookies need to be adjusted
  17     for this so that the proxy's lock is taken for each server operation. This
  18     is possible because the proxy's server list is built once at boot time and
  19     remains stable. So this is what this patch does.
  20
  21     The comments in the proxy and server structs were updated to mention this
  22     rule that the server's lock may not be taken under the proxy's lock but
  23     may enclose it.
  24
  25     Another approach could consist in using a second lock for the proxy's queue
  26     which would be different from the regular proxy's lock, but given that the
  27     operations above are rare and operate on small servers list, there is no
  28     reason for overdesigning a solution.
  29
  30     This fix was successfully tested with 10000 servers in a backend where
  31     adjusting the dyncookies in loops over the CLI didn't have a measurable
  32     impact on the traffic.
  33
  34     The only workaround without the fix is to disable any occurrence of
  35     "slowstart" on server lines, or to disable threads using "nbthread 1".
  36
  37     This must be backported as far as 1.8.
  38
  39     (cherry picked from commit 5e83d996cf965ee5ac625f702a446f4d8c80a220)
  40     Signed-off-by: Willy Tarreau <w@1wt.eu>
  41
  42 diff --git a/include/types/proxy.h b/include/types/proxy.h
  43 index ca24dbfe..2518f88d 100644
  44 --- a/include/types/proxy.h
  45 +++ b/include/types/proxy.h
  46 @@ -487,7 +487,7 @@ struct proxy {
  47                                                  * name is used
  48                                                  */
  49         struct list filter_configs;             /* list of the filters that are declared on this proxy */
  50 -       __decl_hathreads(HA_SPINLOCK_T lock);
  51 +       __decl_hathreads(HA_SPINLOCK_T lock);   /* may be taken under the server's lock */
  52  };
  53
  54  struct switching_rule {
  55 diff --git a/include/types/server.h b/include/types/server.h
  56 index 4a077268..e0534162 100644
  57 --- a/include/types/server.h
  58 +++ b/include/types/server.h
  59 @@ -319,7 +319,7 @@ struct server {
  60         } ssl_ctx;
  61  #endif
  62         struct dns_srvrq *srvrq;                /* Pointer representing the DNS SRV requeest, if any */
  63 -       __decl_hathreads(HA_SPINLOCK_T lock);
  64 +       __decl_hathreads(HA_SPINLOCK_T lock);   /* may enclose the proxy's lock, must not be taken under */
  65         struct {
  66                 const char *file;               /* file where the section appears */
  67                 struct eb32_node id;            /* place in the tree of used IDs */
  68 diff --git a/src/proxy.c b/src/proxy.c
  69 index ae761ead..a537e0b1 100644
  70 --- a/src/proxy.c
  71 +++ b/src/proxy.c
  72 @@ -1940,9 +1940,12 @@ static int cli_parse_enable_dyncookie_backend(char **args, char *payload, struct
  73         if (!px)
  74                 return 1;
  75
  76 +       /* Note: this lock is to make sure this doesn't change while another
  77 +        * thread is in srv_set_dyncookie().
  78 +        */
  79         HA_SPIN_LOCK(PROXY_LOCK, &px->lock);
  80 -
  81         px->ck_opts |= PR_CK_DYNAMIC;
  82 +       HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
  83
  84         for (s = px->srv; s != NULL; s = s->next) {
  85                 HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
  86 @@ -1950,8 +1953,6 @@ static int cli_parse_enable_dyncookie_backend(char **args, char *payload, struct
  87                 HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
  88         }
  89
  90 -       HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
  91 -
  92         return 1;
  93  }
  94
  95 @@ -1971,9 +1972,12 @@ static int cli_parse_disable_dyncookie_backend(char **args, char *payload, struc
  96         if (!px)
  97                 return 1;
  98
  99 +       /* Note: this lock is to make sure this doesn't change while another
 100 +        * thread is in srv_set_dyncookie().
 101 +        */
 102         HA_SPIN_LOCK(PROXY_LOCK, &px->lock);
 103 -
 104         px->ck_opts &= ~PR_CK_DYNAMIC;
 105 +       HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
 106
 107         for (s = px->srv; s != NULL; s = s->next) {
 108                 HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
 109 @@ -1984,8 +1988,6 @@ static int cli_parse_disable_dyncookie_backend(char **args, char *payload, struc
 110                 HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 111         }
 112
 113 -       HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
 114 -
 115         return 1;
 116  }
 117
 118 @@ -2021,10 +2023,13 @@ static int cli_parse_set_dyncookie_key_backend(char **args, char *payload, struc
 119                 return 1;
 120         }
 121
 122 +       /* Note: this lock is to make sure this doesn't change while another
 123 +        * thread is in srv_set_dyncookie().
 124 +        */
 125         HA_SPIN_LOCK(PROXY_LOCK, &px->lock);
 126 -
 127         free(px->dyncookie_key);
 128         px->dyncookie_key = newkey;
 129 +       HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
 130
 131         for (s = px->srv; s != NULL; s = s->next) {
 132                 HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
 133 @@ -2032,8 +2037,6 @@ static int cli_parse_set_dyncookie_key_backend(char **args, char *payload, struc
 134                 HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 135         }
 136
 137 -       HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
 138 -
 139         return 1;
 140  }
 141
 142 diff --git a/src/queue.c b/src/queue.c
 143 index f4a94530..6aa54170 100644
 144 --- a/src/queue.c
 145 +++ b/src/queue.c
 146 @@ -312,16 +312,16 @@ void process_srv_queue(struct server *s)
 147         struct proxy  *p = s->proxy;
 148         int maxconn;
 149
 150 -       HA_SPIN_LOCK(PROXY_LOCK,  &p->lock);
 151         HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
 152 +       HA_SPIN_LOCK(PROXY_LOCK,  &p->lock);
 153         maxconn = srv_dynamic_maxconn(s);
 154         while (s->served < maxconn) {
 155                 int ret = pendconn_process_next_strm(s, p);
 156                 if (!ret)
 157                         break;
 158         }
 159 -       HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 160         HA_SPIN_UNLOCK(PROXY_LOCK,  &p->lock);
 161 +       HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 162  }
 163
 164  /* Adds the stream <strm> to the pending connection queue of server <strm>->srv
 165 @@ -424,7 +424,8 @@ int pendconn_redistribute(struct server *s)
 166  /* Check for pending connections at the backend, and assign some of them to
 167   * the server coming up. The server's weight is checked before being assigned
 168   * connections it may not be able to handle. The total number of transferred
 169 - * connections is returned.
 170 + * connections is returned. It must be called with the server lock held, and
 171 + * will take the proxy's lock.
 172   */
 173  int pendconn_grab_from_px(struct server *s)
 174  {
 175 diff --git a/src/server.c b/src/server.c
 176 index a96f1ef6..236d6bae 100644
 177 --- a/src/server.c
 178 +++ b/src/server.c
 179 @@ -125,7 +125,7 @@ static inline void srv_check_for_dup_dyncookie(struct server *s)
 180  }
 181
 182  /*
 183 - * Must be called with the server lock held.
 184 + * Must be called with the server lock held, and will grab the proxy lock.
 185   */
 186  void srv_set_dyncookie(struct server *s)
 187  {
 188 @@ -137,15 +137,17 @@ void srv_set_dyncookie(struct server *s)
 189         int addr_len;
 190         int port;
 191
 192 +       HA_SPIN_LOCK(PROXY_LOCK, &p->lock);
 193 +
 194         if ((s->flags & SRV_F_COOKIESET) ||
 195             !(s->proxy->ck_opts & PR_CK_DYNAMIC) ||
 196             s->proxy->dyncookie_key == NULL)
 197 -               return;
 198 +               goto out;
 199         key_len = strlen(p->dyncookie_key);
 200
 201         if (s->addr.ss_family != AF_INET &&
 202             s->addr.ss_family != AF_INET6)
 203 -               return;
 204 +               goto out;
 205         /*
 206          * Buffer to calculate the cookie value.
 207          * The buffer contains the secret key + the server IP address
 208 @@ -174,7 +176,7 @@ void srv_set_dyncookie(struct server *s)
 209         hash_value = XXH64(tmpbuf, buffer_len, 0);
 210         memprintf(&s->cookie, "%016llx", hash_value);
 211         if (!s->cookie)
 212 -               return;
 213 +               goto out;
 214         s->cklen = 16;
 215
 216         /* Don't bother checking if the dyncookie is duplicated if
 217 @@ -183,6 +185,8 @@ void srv_set_dyncookie(struct server *s)
 218          */
 219         if (!(s->next_admin & SRV_ADMF_FMAINT))
 220                 srv_check_for_dup_dyncookie(s);
 221 + out:
 222 +       HA_SPIN_UNLOCK(PROXY_LOCK, &p->lock);
 223  }
 224
 225  /*