From 1b87748ff5fe98f230f9cde2a754243d1da4202e Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 21 Aug 2018 19:44:53 +0200 Subject: [PATCH] BUG/MEDIUM: lb/threads: always properly lock LB algorithms on maintenance operations Since commit 3ff577e ("MAJOR: server: make server state changes synchronous again"), srv_update_status() calls the various maintenance operations of the LB algorithms (->set_server_up, ->set_server_down, ->update_server_weight()). These ones are called with a single thread guaranteed by the rendez-vous point, so the fact that they're lacking some locks has no effect. However we'll need to remove the rendez-vous point so we have to take care of properly locking all the LB algos. The comments have been properly updated on the various functions to mention their locking expectations. All these functions are called with the server lock held, and all of them now support concurrent calls by using the lbprm's lock. This fix doesn't need to be backported at the moment, though if any check-specific issue surfaced in 1.8, it could make sense to reuse it. --- src/lb_chash.c | 42 ++++++++++++++++++++++++++++++----- src/lb_fas.c | 31 +++++++++++++++++++++++++- src/lb_fwlc.c | 31 +++++++++++++++++++++++++- src/lb_fwrr.c | 59 +++++++++++++++++++++++++++++++++++++++++++++----- src/lb_map.c | 30 ++++++++++++++++++++----- 5 files changed, 174 insertions(+), 19 deletions(-) diff --git a/src/lb_chash.c b/src/lb_chash.c index e3bf65dcc..7e188ac41 100644 --- a/src/lb_chash.c +++ b/src/lb_chash.c @@ -66,6 +66,8 @@ static inline void chash_dequeue_srv(struct server *s) * as many times as its weight indicates it. If it's there too often, we remove * the last occurrences. If it's not there enough, we add more occurrences. To * remove a server from the tree, normally call this with eweight=0. + * + * The server's lock and the lbprm's lock must be held. */ static inline void chash_queue_dequeue_srv(struct server *s) { @@ -112,6 +114,8 @@ static inline void chash_queue_dequeue_srv(struct server *s) * It is not important whether the server was already down or not. It is not * important either that the new state is completely down (the caller may not * know all the variables of a server's state). + * + * The server's lock must be held. The lbprm lock will be used. */ static void chash_set_server_status_down(struct server *srv) { @@ -120,6 +124,8 @@ static void chash_set_server_status_down(struct server *srv) if (!srv_lb_status_changed(srv)) return; + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (srv_willbe_usable(srv)) goto out_update_state; @@ -155,6 +161,8 @@ out_update_backend: update_backend_weight(p); out_update_state: srv_lb_commit_status(srv); + + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); } /* This function updates the server trees according to server 's new @@ -163,6 +171,8 @@ out_update_backend: * important either that the new state is completely UP (the caller may not * know all the variables of a server's state). This function will not change * the weight of a server which was already up. + * + * The server's lock must be held. The lbprm lock will be used. */ static void chash_set_server_status_up(struct server *srv) { @@ -171,6 +181,8 @@ static void chash_set_server_status_up(struct server *srv) if (!srv_lb_status_changed(srv)) return; + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (!srv_willbe_usable(srv)) goto out_update_state; @@ -211,10 +223,14 @@ static void chash_set_server_status_up(struct server *srv) update_backend_weight(p); out_update_state: srv_lb_commit_status(srv); + + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); } /* This function must be called after an update to server 's effective * weight. It may be called after a state change too. + * + * The server's lock must be held. The lbprm lock may be used. */ static void chash_update_server_weight(struct server *srv) { @@ -248,6 +264,8 @@ static void chash_update_server_weight(struct server *srv) return; } + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + /* only adjust the server's presence in the tree */ chash_queue_dequeue_srv(srv); @@ -258,6 +276,8 @@ static void chash_update_server_weight(struct server *srv) update_backend_weight(p); srv_lb_commit_status(srv); + + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); } /* @@ -303,21 +323,29 @@ struct server *chash_get_server_hash(struct proxy *p, unsigned int hash) unsigned int dn, dp; int loop; + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (p->srv_act) root = &p->lbprm.chash.act; - else if (p->lbprm.fbck) - return p->lbprm.fbck; + else if (p->lbprm.fbck) { + nsrv = p->lbprm.fbck; + goto out; + } else if (p->srv_bck) root = &p->lbprm.chash.bck; - else - return NULL; + else { + nsrv = NULL; + goto out; + } /* find the node after and the node before */ next = eb32_lookup_ge(root, hash); if (!next) next = eb32_first(root); - if (!next) - return NULL; /* tree is empty */ + if (!next) { + nsrv = NULL; /* tree is empty */ + goto out; + } prev = eb32_prev(next); if (!prev) @@ -349,6 +377,8 @@ struct server *chash_get_server_hash(struct proxy *p, unsigned int hash) nsrv = eb32_entry(next, struct tree_occ, node)->server; } + out: + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); return nsrv; } diff --git a/src/lb_fas.c b/src/lb_fas.c index d30114369..69b85d72d 100644 --- a/src/lb_fas.c +++ b/src/lb_fas.c @@ -31,13 +31,18 @@ /* Remove a server from a tree. It must have previously been dequeued. This * function is meant to be called when a server is going down or has its * weight disabled. + * + * The server's lock and the lbprm's lock must be held. */ static inline void fas_remove_from_tree(struct server *s) { s->lb_tree = NULL; } -/* simply removes a server from a tree */ +/* simply removes a server from a tree. + * + * The server's lock and the lbprm's lock must be held. + */ static inline void fas_dequeue_srv(struct server *s) { eb32_delete(&s->lb_node); @@ -48,6 +53,8 @@ static inline void fas_dequeue_srv(struct server *s) * available server in declaration order (or ID order) until its maxconn is * reached. It is important to understand that the server weight is not used * here. + * + * The server's lock and the lbprm's lock must be held. */ static inline void fas_queue_srv(struct server *s) { @@ -58,6 +65,8 @@ static inline void fas_queue_srv(struct server *s) /* Re-position the server in the FS tree after it has been assigned one * connection or after it has released one. Note that it is possible that * the server has been moved out of the tree due to failed health-checks. + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fas_srv_reposition(struct server *s) { @@ -75,6 +84,8 @@ static void fas_srv_reposition(struct server *s) * It is not important whether the server was already down or not. It is not * important either that the new state is completely down (the caller may not * know all the variables of a server's state). + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fas_set_server_status_down(struct server *srv) { @@ -86,6 +97,8 @@ static void fas_set_server_status_down(struct server *srv) if (srv_willbe_usable(srv)) goto out_update_state; + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (!srv_currently_usable(srv)) /* server was already down */ goto out_update_backend; @@ -117,6 +130,8 @@ static void fas_set_server_status_down(struct server *srv) out_update_backend: /* check/update tot_used, tot_weight */ update_backend_weight(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); + out_update_state: srv_lb_commit_status(srv); } @@ -127,6 +142,8 @@ static void fas_set_server_status_down(struct server *srv) * important either that the new state is completely UP (the caller may not * know all the variables of a server's state). This function will not change * the weight of a server which was already up. + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fas_set_server_status_up(struct server *srv) { @@ -138,6 +155,8 @@ static void fas_set_server_status_up(struct server *srv) if (!srv_willbe_usable(srv)) goto out_update_state; + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (srv_currently_usable(srv)) /* server was already up */ goto out_update_backend; @@ -175,12 +194,16 @@ static void fas_set_server_status_up(struct server *srv) out_update_backend: /* check/update tot_used, tot_weight */ update_backend_weight(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); + out_update_state: srv_lb_commit_status(srv); } /* This function must be called after an update to server 's effective * weight. It may be called after a state change too. + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fas_update_server_weight(struct server *srv) { @@ -214,6 +237,8 @@ static void fas_update_server_weight(struct server *srv) return; } + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (srv->lb_tree) fas_dequeue_srv(srv); @@ -228,6 +253,8 @@ static void fas_update_server_weight(struct server *srv) fas_queue_srv(srv); update_backend_weight(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); + srv_lb_commit_status(srv); } @@ -269,6 +296,8 @@ void fas_init_server_tree(struct proxy *p) /* Return next server from the FS tree in backend

. If the tree is empty, * return NULL. Saturated servers are skipped. + * + * The server's lock must be held. The lbprm's lock will be used. */ struct server *fas_get_next_server(struct proxy *p, struct server *srvtoavoid) { diff --git a/src/lb_fwlc.c b/src/lb_fwlc.c index fd9b437c1..ca422217e 100644 --- a/src/lb_fwlc.c +++ b/src/lb_fwlc.c @@ -25,13 +25,18 @@ /* Remove a server from a tree. It must have previously been dequeued. This * function is meant to be called when a server is going down or has its * weight disabled. + * + * The server's lock and the lbprm's lock must be held. */ static inline void fwlc_remove_from_tree(struct server *s) { s->lb_tree = NULL; } -/* simply removes a server from a tree */ +/* simply removes a server from a tree. + * + * The server's lock and the lbprm's lock must be held. + */ static inline void fwlc_dequeue_srv(struct server *s) { eb32_delete(&s->lb_node); @@ -40,6 +45,8 @@ static inline void fwlc_dequeue_srv(struct server *s) /* Queue a server in its associated tree, assuming the weight is >0. * Servers are sorted by #conns/weight. To ensure maximum accuracy, * we use #conns*SRV_EWGHT_MAX/eweight as the sorting key. + * + * The server's lock and the lbprm's lock must be held. */ static inline void fwlc_queue_srv(struct server *s) { @@ -50,6 +57,8 @@ static inline void fwlc_queue_srv(struct server *s) /* Re-position the server in the FWLC tree after it has been assigned one * connection or after it has released one. Note that it is possible that * the server has been moved out of the tree due to failed health-checks. + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fwlc_srv_reposition(struct server *s) { @@ -67,6 +76,8 @@ static void fwlc_srv_reposition(struct server *s) * It is not important whether the server was already down or not. It is not * important either that the new state is completely down (the caller may not * know all the variables of a server's state). + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fwlc_set_server_status_down(struct server *srv) { @@ -77,6 +88,8 @@ static void fwlc_set_server_status_down(struct server *srv) if (srv_willbe_usable(srv)) goto out_update_state; + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (!srv_currently_usable(srv)) /* server was already down */ @@ -109,6 +122,8 @@ static void fwlc_set_server_status_down(struct server *srv) out_update_backend: /* check/update tot_used, tot_weight */ update_backend_weight(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); + out_update_state: srv_lb_commit_status(srv); } @@ -119,6 +134,8 @@ out_update_backend: * important either that the new state is completely UP (the caller may not * know all the variables of a server's state). This function will not change * the weight of a server which was already up. + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fwlc_set_server_status_up(struct server *srv) { @@ -130,6 +147,8 @@ static void fwlc_set_server_status_up(struct server *srv) if (!srv_willbe_usable(srv)) goto out_update_state; + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (srv_currently_usable(srv)) /* server was already up */ goto out_update_backend; @@ -167,12 +186,16 @@ static void fwlc_set_server_status_up(struct server *srv) out_update_backend: /* check/update tot_used, tot_weight */ update_backend_weight(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); + out_update_state: srv_lb_commit_status(srv); } /* This function must be called after an update to server 's effective * weight. It may be called after a state change too. + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fwlc_update_server_weight(struct server *srv) { @@ -206,6 +229,8 @@ static void fwlc_update_server_weight(struct server *srv) return; } + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (srv->lb_tree) fwlc_dequeue_srv(srv); @@ -220,6 +245,8 @@ static void fwlc_update_server_weight(struct server *srv) fwlc_queue_srv(srv); update_backend_weight(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); + srv_lb_commit_status(srv); } @@ -261,6 +288,8 @@ void fwlc_init_server_tree(struct proxy *p) /* Return next server from the FWLC tree in backend

. If the tree is empty, * return NULL. Saturated servers are skipped. + * + * The server's lock must be held. The lbprm's lock will be used. */ struct server *fwlc_get_next_server(struct proxy *p, struct server *srvtoavoid) { diff --git a/src/lb_fwrr.c b/src/lb_fwrr.c index cba7db5f0..73a140b86 100644 --- a/src/lb_fwrr.c +++ b/src/lb_fwrr.c @@ -33,6 +33,8 @@ static void fwrr_queue_srv(struct server *s); * It is not important whether the server was already down or not. It is not * important either that the new state is completely down (the caller may not * know all the variables of a server's state). + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fwrr_set_server_status_down(struct server *srv) { @@ -45,6 +47,8 @@ static void fwrr_set_server_status_down(struct server *srv) if (srv_willbe_usable(srv)) goto out_update_state; + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (!srv_currently_usable(srv)) /* server was already down */ goto out_update_backend; @@ -79,6 +83,8 @@ static void fwrr_set_server_status_down(struct server *srv) out_update_backend: /* check/update tot_used, tot_weight */ update_backend_weight(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); + out_update_state: srv_lb_commit_status(srv); } @@ -89,6 +95,8 @@ out_update_backend: * important either that the new state is completely UP (the caller may not * know all the variables of a server's state). This function will not change * the weight of a server which was already up. + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fwrr_set_server_status_up(struct server *srv) { @@ -101,6 +109,8 @@ static void fwrr_set_server_status_up(struct server *srv) if (!srv_willbe_usable(srv)) goto out_update_state; + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + if (srv_currently_usable(srv)) /* server was already up */ goto out_update_backend; @@ -141,12 +151,16 @@ static void fwrr_set_server_status_up(struct server *srv) out_update_backend: /* check/update tot_used, tot_weight */ update_backend_weight(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); + out_update_state: srv_lb_commit_status(srv); } /* This function must be called after an update to server 's effective * weight. It may be called after a state change too. + * + * The server's lock must be held. The lbprm's lock will be used. */ static void fwrr_update_server_weight(struct server *srv) { @@ -181,6 +195,8 @@ static void fwrr_update_server_weight(struct server *srv) return; } + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); + grp = (srv->flags & SRV_F_BACKUP) ? &p->lbprm.fwrr.bck : &p->lbprm.fwrr.act; grp->next_weight = grp->next_weight - srv->cur_eweight + srv->next_eweight; @@ -227,12 +243,16 @@ static void fwrr_update_server_weight(struct server *srv) } update_backend_weight(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); + srv_lb_commit_status(srv); } /* Remove a server from a tree. It must have previously been dequeued. This * function is meant to be called when a server is going down or has its * weight disabled. + * + * The server's lock and the lbprm's lock must be held. */ static inline void fwrr_remove_from_tree(struct server *s) { @@ -242,6 +262,8 @@ static inline void fwrr_remove_from_tree(struct server *s) /* Queue a server in the weight tree , assuming the weight is >0. * We want to sort them by inverted weights, because we need to place * heavy servers first in order to get a smooth distribution. + * + * The server's lock and the lbprm's lock must be held. */ static inline void fwrr_queue_by_weight(struct eb_root *root, struct server *s) { @@ -299,7 +321,10 @@ void fwrr_init_server_groups(struct proxy *p) } } -/* simply removes a server from a weight tree */ +/* simply removes a server from a weight tree. + * + * The server's lock and the lbprm's lock must be held. + */ static inline void fwrr_dequeue_srv(struct server *s) { eb32_delete(&s->lb_node); @@ -308,6 +333,8 @@ static inline void fwrr_dequeue_srv(struct server *s) /* queues a server into the appropriate group and tree depending on its * backup status, and ->npos. If the server is disabled, simply assign * it to the NULL tree. + * + * The server's lock and the lbprm's lock must be held. */ static void fwrr_queue_srv(struct server *s) { @@ -346,13 +373,19 @@ static void fwrr_queue_srv(struct server *s) } } -/* prepares a server when extracting it from the "init" tree */ +/* prepares a server when extracting it from the "init" tree. + * + * The server's lock and the lbprm's lock must be held. + */ static inline void fwrr_get_srv_init(struct server *s) { s->npos = s->rweight = 0; } -/* prepares a server when extracting it from the "next" tree */ +/* prepares a server when extracting it from the "next" tree. + * + * The server's lock and the lbprm's lock must be held. + */ static inline void fwrr_get_srv_next(struct server *s) { struct fwrr_group *grp = (s->flags & SRV_F_BACKUP) ? @@ -362,7 +395,10 @@ static inline void fwrr_get_srv_next(struct server *s) HA_ATOMIC_ADD(&s->npos, grp->curr_weight); } -/* prepares a server when it was marked down */ +/* prepares a server when it was marked down. + * + * The server's lock and the lbprm's lock must be held. + */ static inline void fwrr_get_srv_down(struct server *s) { struct fwrr_group *grp = (s->flags & SRV_F_BACKUP) ? @@ -372,7 +408,10 @@ static inline void fwrr_get_srv_down(struct server *s) s->npos = grp->curr_pos; } -/* prepares a server when extracting it from its tree */ +/* prepares a server when extracting it from its tree. + * + * The server's lock and the lbprm's lock must be held. + */ static void fwrr_get_srv(struct server *s) { struct proxy *p = s->proxy; @@ -393,6 +432,8 @@ static void fwrr_get_srv(struct server *s) /* switches trees "init" and "next" for FWRR group . "init" should be empty * when this happens, and "next" filled with servers sorted by weights. + * + * The lbprm's lock must be held. */ static inline void fwrr_switch_trees(struct fwrr_group *grp) { @@ -406,6 +447,8 @@ static inline void fwrr_switch_trees(struct fwrr_group *grp) /* return next server from the current tree in FWRR group , or a server * from the "init" tree if appropriate. If both trees are empty, return NULL. + * + * The lbprm's lock must be held. */ static struct server *fwrr_get_server_from_group(struct fwrr_group *grp) { @@ -435,7 +478,9 @@ static struct server *fwrr_get_server_from_group(struct fwrr_group *grp) /* Computes next position of server in the group. It is mandatory for * to have a non-zero, positive eweight. -*/ + * + * The server's lock and the lbprm's lock must be held. + */ static inline void fwrr_update_position(struct fwrr_group *grp, struct server *s) { if (!s->npos) { @@ -463,6 +508,8 @@ static inline void fwrr_update_position(struct fwrr_group *grp, struct server *s /* Return next server from the current tree in backend

, or a server from * the init tree if appropriate. If both trees are empty, return NULL. * Saturated servers are skipped and requeued. + * + * The server's lock must be held. The lbprm's lock will be used. */ struct server *fwrr_get_next_server(struct proxy *p, struct server *srvtoavoid) { diff --git a/src/lb_map.c b/src/lb_map.c index a1e1d3492..54569b0a2 100644 --- a/src/lb_map.c +++ b/src/lb_map.c @@ -24,7 +24,10 @@ #include #include -/* this function updates the map according to server 's new state */ +/* this function updates the map according to server 's new state. + * + * The server's lock must be held. The lbprm's lock will be used. + */ static void map_set_server_status_down(struct server *srv) { struct proxy *p = srv->proxy; @@ -36,14 +39,19 @@ static void map_set_server_status_down(struct server *srv) goto out_update_state; /* FIXME: could be optimized since we know what changed */ + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); recount_servers(p); update_backend_weight(p); recalc_server_map(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); out_update_state: srv_lb_commit_status(srv); } -/* This function updates the map according to server 's new state */ +/* This function updates the map according to server 's new state. + * + * The server's lock must be held. The lbprm's lock will be used. + */ static void map_set_server_status_up(struct server *srv) { struct proxy *p = srv->proxy; @@ -55,9 +63,11 @@ static void map_set_server_status_up(struct server *srv) goto out_update_state; /* FIXME: could be optimized since we know what changed */ + HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock); recount_servers(p); update_backend_weight(p); recalc_server_map(p); + HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock); out_update_state: srv_lb_commit_status(srv); } @@ -66,6 +76,8 @@ static void map_set_server_status_up(struct server *srv) * px->lbprm.tot_wact, tot_wbck, tot_used, tot_weight, so it must be * called after recount_servers(). It also expects px->lbprm.map.srv * to be allocated with the largest size needed. It updates tot_weight. + * + * The lbprm's lock must be held. */ void recalc_server_map(struct proxy *px) { @@ -202,6 +214,8 @@ void init_server_map(struct proxy *p) * the proxy following the round-robin method. * If any server is found, it will be returned and px->lbprm.map.rr_idx will be updated * to point to the next server. If no valid server is found, NULL is returned. + * + * The lbprm's lock will be used. */ struct server *map_get_server_rr(struct proxy *px, struct server *srvtoavoid) { @@ -250,12 +264,18 @@ struct server *map_get_server_rr(struct proxy *px, struct server *srvtoavoid) * pointed to by the result of a modulo operation on . The server map may * be recomputed if required before being looked up. If any server is found, it * will be returned. If no valid server is found, NULL is returned. + * + * The lbprm's lock will be used. */ struct server *map_get_server_hash(struct proxy *px, unsigned int hash) { - if (px->lbprm.tot_weight == 0) - return NULL; - return px->lbprm.map.srv[hash % px->lbprm.tot_weight]; + struct server *srv = NULL; + + HA_SPIN_LOCK(LBPRM_LOCK, &px->lbprm.lock); + if (px->lbprm.tot_weight) + srv = px->lbprm.map.srv[hash % px->lbprm.tot_weight]; + HA_SPIN_UNLOCK(LBPRM_LOCK, &px->lbprm.lock); + return srv; }