On 2013/8/6 20:36, Willy Tarreau wrote:
> Hi Godbach,
> 
> Sorry for replying late, I don't have a regular internet access at the
> moment.
> 
> On Tue, Aug 06, 2013 at 07:48:46PM +0800, Godbach wrote:
>> It seems that static-rr should also use the same check condition for the
>> server after being selected as roundrobin and leastconn as below:
>>
>> if (!srv->maxconn || (!srv->nbpend && srv->served <
>> srv_dynamic_maxconn(srv)))
>>
>> not the following:
>>
>> if (!srv->maxconn || srv->cur_sess < srv_dynamic_maxconn(srv)) {
> 
> Yes I agree with you, I think the reason is that the code was
> extracted from the hash codes (which also fall back to RR if
> the content to hash is not here), and which initially did not
> need to handle this situation.
> 
> I don't remember if the hash algorithms now fall back to the
> generic RR code or if they use their own, but it is possible
> that more code needs fixing in the end.
> 
> Care to send a patch ? :-)
> 
> Best regards,
> Willy
> 
> 

Hi Willy,

Thank you for your replying.

I have checked all the hash algorithms, if the hash type is consistent,
they will call chash_get_server_hash(), and map_get_server_hash() will
be called with hash type is map-based. Both functions will return server
directly without checking server limition. The check will be executed in
assign_server_and_queue() as below:

err = assign_server(s);
...
switch (err) {
...
case SRV_STATUS_OK:

        if (srv->maxconn &&
            (srv->nbpend || srv->served >= srv_dynamic_maxconn(srv))) {

                if (srv->maxqueue > 0 && srv->nbpend >= srv->maxqueue)
                        return SRV_STATUS_FULL;

                p = pendconn_add(s);
                if (p)
                        return SRV_STATUS_QUEUED;
                else
                        return SRV_STATUS_INTERNAL;
        }
...

So in my opinion, it is not neccessary to worry about hash algorithms.

The attachment is the patch which fix the check condition for static-rr
in map_get_server_rr() for your information. This patch will only affect
static-rr. Though all hash algorithms with type map-based will use the
same server map as static-rr, they call another function
map_get_server_hash() to get server.

By the way, it seems that hash type avalanche was not used anymore since
there are only definition and parsing for it in lastest snapshot as below:

src/cfgparse.c
4101:                   curproxy->lbprm.algo |= BE_LB_HASH_AVAL;

include/types/backend.h
108:#define BE_LB_HASH_AVAL   0x200000 /* run an avalanche hash before a
map */

Is there anything I missed?

-- 
Best Regards,
Godbach
From 370a74e89af0153a96ed8b7ebd4648258c89109e Mon Sep 17 00:00:00 2001
From: Godbach <nylzhao...@gmail.com>
Date: Wed, 7 Aug 2013 09:48:23 +0800
Subject: [PATCH] BUG/MINOR: use the same check condition for server as other
 algorithms

Such load balance algorithms as roundrobin, leastconn and first will check the
server after being selected with the following condition:
        if (!s->maxconn || (!s->nbpend && s->served < srv_dynamic_maxconn(s)))

But static-rr uses the different one in map_get_server_rr()  as below:
        if (!srv->maxconn || srv->cur_sess < srv_dynamic_maxconn(srv))
After viewing this difference, it is a better choice for static-rr to use the
same check condition as other algorithms.

This change will only affect static-rr. Though all hash algorithms with type
map-based will use the same server map as static-rr, they call another function
map_get_server_hash() to get server.

Signed-off-by: Godbach <nylzhao...@gmail.com>
---
 src/lb_map.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/lb_map.c b/src/lb_map.c
index 49805ad..9858249 100644
--- a/src/lb_map.c
+++ b/src/lb_map.c
@@ -229,7 +229,7 @@ struct server *map_get_server_rr(struct proxy *px, struct 
server *srvtoavoid)
        avoididx = 0; /* shut a gcc warning */
        do {
                srv = px->lbprm.map.srv[newidx++];
-               if (!srv->maxconn || srv->cur_sess < srv_dynamic_maxconn(srv)) {
+               if (!srv->maxconn || (!srv->nbpend && srv->served < 
srv_dynamic_maxconn(srv))) {
                        /* make sure it is not the server we are try to 
exclude... */
                        if (srv != srvtoavoid) {
                                px->lbprm.map.rr_idx = newidx;
-- 
1.7.7

Reply via email to