Hi Aleks, On Fri, Feb 06, 2026 at 08:48:06AM +0100, Aleksandar Lazic wrote: > Hi. > > On 2026-02-06 (Fr.) 01:02, Aleksandar Lazic wrote: > > Hi. > > > > here my first attempt to add the (Peak) EWMA to HAProxy. > > In the first post was the doc and reg-test missing this patches are now > added in that mail. > Regards > Aleks
> From 7db40d8b38865f0bb065aae09b7f3f12d36e582c Mon Sep 17 00:00:00 2001 > From: Aleksandar Lazic <[email protected]> > Date: Fri, 6 Feb 2026 08:45:32 +0100 > Subject: [PATCH 10/10] MEDIUM/backend: Add Peak EWMA load balancing algorithm > > refer to https://github.com/haproxy/haproxy/issues/1570 > > Signed-off-by: Aleksandar Lazic <[email protected]> (...) Well, it's impossible to review like this honestly, all commit messages are exactly the same and there is zero info in them. The principles of commit messages are: - they must be descriptive of the patch itself: what does the patch bring, what were the architectural choices and principles, why is it considered a good approach, whether or not other approaches were considered and evicted, what tradeoffs were made, whether it can bring improvements as well as degradations in some specific cases, etc. As you know, I'm used to saying that these are all the arguments to "sell" the patch and ask someone to merge it and maintain it forever. - only include external links for complementary information (e.g. a link to a paper). External links to figure the purpose of the patch are definitely not acceptable because 1) you can't review nor debug offline, and 2) you're at the mercy of the site hosting the contents. - subject lines *should* be unique in a project. Note that I'm not saying "must" because accidents happen, but they should be specific enough so that the risk of conflict remains almost zero. The one above for example could be "DOC: config: document the new peak-ewma load-balancing algorithm". - when relevant, please also indicate the conditions in which this was tested (and if possible what was observed, i.e. measures etc). This is particularly important when someone starts to face issues in uncommon environments and it's unknown whether this is getting out of the initial scope. I'll try to review the rest later (maybe start next week, but really, without any intent in the commit messages to know what you're reviewing, it's extremely difficult). I'm seeing a few things below upon a quick glance though: > diff --git a/src/lb_pewma.c b/src/lb_pewma.c > new file mode 100644 > index 0000000000..69ac79fbe9 > --- /dev/null > +++ b/src/lb_pewma.c > @@ -0,0 +1,938 @@ (...) > +/* We reuse the same tree element layout as FWLC. The struct fwlc_tree_elt > + * type is forward-declared in server-t.h via pointer members. We provide > + * the full definition here so that we can access its fields. > + */ > +struct fwlc_tree_elt { > + struct mt_list srv_list[PEWMA_LISTS_NB]; > + struct mt_list free_list; > + struct eb32_node lb_node; > + unsigned int elements; > +}; Please never ever redefine a struct somewhere else. It's granted to break at the very first change that the other definition will experience. Either you need it and you include the relevant include files, or you define your own. Note that I strongly doubt that you'd need to reuse the same though, and if it is really connection oriented, then it must absolutely be tested under load, because leastconn used to perform extremely badly in terms of CPU usage on modern systems, forcing us to invest a lot of thinking into how to redesign it to strongly reduce the amount of sharing between CPUs. And forcefully sharing types between different things can just condemn both algorithms by making it too hard to make one evolve without breaking the other one. > @@ -58,6 +59,7 @@ > /* BE_LB_CB_* is used with BE_LB_KIND_CB */ > #define BE_LB_CB_LC 0x00000000 /* least-connections */ > #define BE_LB_CB_FAS 0x00000001 /* first available server (opposite of > leastconn) */ > +#define BE_LB_CB_PE 0x00000002 /* peak ewma */ > > /* BE_LB_SA_* is used with BE_LB_KIND_SA */ > #define BE_LB_SA_SS 0x00000000 /* stick to server as long as it is > available */ > @@ -88,6 +90,7 @@ > #define BE_LB_ALGO_RND (BE_LB_KIND_RR | BE_LB_NEED_NONE | BE_LB_RR_RANDOM) > /* random value */ > #define BE_LB_ALGO_LC (BE_LB_KIND_CB | BE_LB_NEED_NONE | BE_LB_CB_LC) > /* least connections */ > #define BE_LB_ALGO_FAS (BE_LB_KIND_CB | BE_LB_NEED_NONE | BE_LB_CB_FAS) > /* first available server */ > +#define BE_LB_ALGO_PE (BE_LB_KIND_CB | BE_LB_NEED_NONE | BE_LB_CB_PE) > /* peak ewma */ > #define BE_LB_ALGO_SS (BE_LB_KIND_SA | BE_LB_NEED_NONE | BE_LB_SA_SS) > /* sticky */ > #define BE_LB_ALGO_SRR (BE_LB_KIND_RR | BE_LB_NEED_NONE | BE_LB_RR_STATIC) > /* static round robin */ > #define BE_LB_ALGO_SH (BE_LB_KIND_HI | BE_LB_NEED_ADDR | > BE_LB_HASH_SRC) /* hash: source IP */ (...) So if it's in _CB_ it's connection-based (like leastconn thus highly sensitive to CPU count). Otherwise if it's like random or round robin, it'd rather be set as KIND_RR (round robin, i.e. no dependency on the connection count). Thanks, Willy
