Re: [PATCH 1/1: BUILD/MINOR: TCP_KEEPIDLE macos equivalence
On Sun, May 08, 2022 at 12:39:13PM +0200, Vincent Bernat wrote: > ? 8 May 2022 10:57 +02, Willy Tarreau: > > > After edition (still minimal and possibly inaccurate but the best I > > could do): > > > > On Linux the interval before starting to send TCP keep-alive packets > > is defined by TCP_KEEPIDLE. MacOS has an equivalent with TCP_KEEPIDLE, > > which also uses seconds as a unit, so it's possible to simply remap the > > definition of TCP_KEEPIDLE to TCP_KEEPALIVE there and get it to > > seamlessly > > work. The other settings (interval and count) are not present, > > though. > > First instance of TCP_KEEPIDLE should be TCP_KEEPALIVE. ;-) Oops, thanks, that's exactly why I hate having to document others' work myself. It's so easy to get something wrong :-/ Willy
Re: [PATCH 1/1: BUILD/MINOR: TCP_KEEPIDLE macos equivalence
❦ 8 May 2022 10:57 +02, Willy Tarreau: > After edition (still minimal and possibly inaccurate but the best I > could do): > > On Linux the interval before starting to send TCP keep-alive packets > is defined by TCP_KEEPIDLE. MacOS has an equivalent with TCP_KEEPIDLE, > which also uses seconds as a unit, so it's possible to simply remap the > definition of TCP_KEEPIDLE to TCP_KEEPALIVE there and get it to seamlessly > work. The other settings (interval and count) are not present, > though. First instance of TCP_KEEPIDLE should be TCP_KEEPALIVE. ;-) -- Treat end of file conditions in a uniform manner. - The Elements of Programming Style (Kernighan & Plauger)
Re: [PATCH 1/1: BUILD/MINOR: TCP_KEEPIDLE macos equivalence
On Sun, May 08, 2022 at 10:21:28AM +0100, David CARLIER wrote: > On Sun, 8 May 2022 at 09:57, Willy Tarreau wrote: > > > > On Sun, May 01, 2022 at 03:33:17PM +0100, David CARLIER wrote: > > > Hi here a little patch to set idle time for SO_KEEPALIVE socket option. > > > > Now merged, thanks. > > > > David, one comment though, your commit messages keep missing a lot of > > crucial information for reviewers and debuggers, and I had to spend time > > documenting myself on keep-alive on MacOS to figure the differences and/or > > what the impacts or limitations of the patch could be. I finally found and > > put that information into the commit message, but it would be much easier > > if you could put it yourself after your development since you actually > > have access to the information I had to seek, and know the reasoning > > behind your choice. > > > > Thanks, fair point :-) I ll take it in account Thanks ;-) > even tough I was certain for this one it would not break anything. Quite frankly, experience tells us we really never know. If MacOS in the future version would define the TCP_KEEPIDLE macro, that would be sufficient to break the build for example. > > The new choice was explained again. Finally, two weeks later I got a > > report of breakage when using external code linked at run time, the > > keyword registration didn't work anymore due to a mistake in this last > > patch. Fortunately, the design decision was explained and I could > > restart from this, figure my mistake and make sure that the 3rd attempt > > at a fix this time addressed all 3 identified use cases: > > Do you have a reasonable numbers of macOs users or is it a rare occurrence ? I really don't know, here it was on the CI. Outside of the CI, breakage is usually reported in this order: - Linux - FreeBSD - MacOS - others, in random order Thus I guess it translates the number of occurrences in field. I suspect that a number of admins have macs and are used to build it locally to test their configs, and that it's probably a non-negligible part of the MacOS reports. But as usual, I could be totally wrong ;-) Willy
Re: [PATCH 1/1: BUILD/MINOR: TCP_KEEPIDLE macos equivalence
On Sun, 8 May 2022 at 09:57, Willy Tarreau wrote:
>
> On Sun, May 01, 2022 at 03:33:17PM +0100, David CARLIER wrote:
> > Hi here a little patch to set idle time for SO_KEEPALIVE socket option.
>
> Now merged, thanks.
>
> David, one comment though, your commit messages keep missing a lot of
> crucial information for reviewers and debuggers, and I had to spend time
> documenting myself on keep-alive on MacOS to figure the differences and/or
> what the impacts or limitations of the patch could be. I finally found and
> put that information into the commit message, but it would be much easier
> if you could put it yourself after your development since you actually
> have access to the information I had to seek, and know the reasoning
> behind your choice.
>
Thanks, fair point :-) I ll take it in account even tough I was
certain for this one it would not break anything.
...
> The new choice was explained again. Finally, two weeks later I got a
> report of breakage when using external code linked at run time, the
> keyword registration didn't work anymore due to a mistake in this last
> patch. Fortunately, the design decision was explained and I could
> restart from this, figure my mistake and make sure that the 3rd attempt
> at a fix this time addressed all 3 identified use cases:
Do you have a reasonable numbers of macOs users or is it a rare occurrence ?
>
> commit 65d9f83794e00e136335348de531167f31d2f39b
> Author: Willy Tarreau
> Date: Tue Apr 26 19:35:38 2022 +0200
>
> BUILD: compiler: properly distinguish weak and global symbols
>
> While weak symbols were finally fixed with commit fb1b6f5bc ("BUILD:
> compiler: use a more portable set of asm(".weak") statements"), it
> was an error to think that initcall symbols were also weak. They must
> not be and they're only global. The reason is that any externally
> linked code loaded as a .so would drop its weak symbols when being
> loaded, hence its initcalls that may contain various function
> registration calls.
>
> The ambiguity came from the fact that we initially reused the initcall's
> HA_GLOBL macro for OSX then generalized it, then turned it to a choice
> between .globl and .weak based on the OS, while in fact we needed a
> macro to define weak symbols.
>
> Let's rename the macro to HA_WEAK() to make it clear it's only for weak
> symbols, and redefine HA_GLOBL() that initcall needs.
>
> This will need to be backported wherever the commit above is backported
> (at least 2.5 for now).
>
I did not notice really those mem stats until now, good to know !
> For sure it's not perfect, and anything can always be improved. But
> that's not the point, the point here is to put info about the intent and
> the approach so that these ones can be preserved or corrected later. And
> that's valid for any patch, even a one-liner.
>
> The impact of your patch is that tcp-keep-alive will start to work on
> MacOS and if someone starts to rely on it and someone else later reverts
> your patch because it causes a build issue on a specific platform and the
> person thinks "oh we don't need this one there, we already have something
> else", by just reverting it they could break the other user's deployment.
>
> That's why it's important to put enough explanation to ensure the patch
> remains firmly stuck to the code base and cannot vanish without an extremly
> good argument.
>
Alrighty :-)
> Hoping this helps!
>
> Cheers
> Willy
Re: [PATCH 1/1: BUILD/MINOR: TCP_KEEPIDLE macos equivalence
On Sun, May 01, 2022 at 03:33:17PM +0100, David CARLIER wrote:
> Hi here a little patch to set idle time for SO_KEEPALIVE socket option.
Now merged, thanks.
David, one comment though, your commit messages keep missing a lot of
crucial information for reviewers and debuggers, and I had to spend time
documenting myself on keep-alive on MacOS to figure the differences and/or
what the impacts or limitations of the patch could be. I finally found and
put that information into the commit message, but it would be much easier
if you could put it yourself after your development since you actually
have access to the information I had to seek, and know the reasoning
behind your choice.
Just an indication, your patch mentioned this:
TCP_KEEPALIVE has the same semantic.
After edition (still minimal and possibly inaccurate but the best I
could do):
On Linux the interval before starting to send TCP keep-alive packets
is defined by TCP_KEEPIDLE. MacOS has an equivalent with TCP_KEEPIDLE,
which also uses seconds as a unit, so it's possible to simply remap the
definition of TCP_KEEPIDLE to TCP_KEEPALIVE there and get it to seamlessly
work. The other settings (interval and count) are not present, though.
A good rule of thumb to write a useful commit message is to think about
these 3 steps:
- first, convince a project maintainer that your work is worth being
merged and is very unlikely to cause trouble elsewhere ;
- second, provide helpful information about why it's done like this and
what it provides, to someone who would see a bisect session finish on
this patch, so that this person doesn't simply think "what's this mess,
let's revert it" without figuring what use case a revert could break ;
- third, by explaining the design decisions, choices and tradeoffs (and
sometimes even alternate solutions that were ditched), you'll help the
person that discovers breakage find a different solution that will
still preserve the original intent while addressing the problem.
For example recently I broke MacOS build while trying to fix a clang
warning. The two patches that caused the breakage were these ones:
commit b12966af1006be8d4438ee1ca39c2541a1f2a4f9
Author: Willy Tarreau
Date: Wed Apr 13 17:09:45 2022 +0200
BUILD: debug: mark the __start_mem_stats/__stop_mem_stats symbols as weak
Building with clang and DEBUG_MEM_STATS shows the following warnings:
warning: __start_mem_stats changed binding to STB_WEAK [-Wsource-mgr]
warning: __stop_mem_stats changed binding to STB_WEAK [-Wsource-mgr]
The reason is that the symbols are declared using ".globl" while they
are also referenced as __attribute__((weak)) elsewhere. It turns out
that a weak symbol is implicitly a global one and that the two classes
are exclusive, thus it may confuse the linker. Better fix this.
This may be backported where the patch applies.
commit 2a06e248f5c8b7c86c7dd48eed7f6d5e87288457
Author: Willy Tarreau
Date: Wed Apr 13 17:12:20 2022 +0200
BUILD: initcall: mark the __start_i_* symbols as weak, not global
Just like for previous fix, these symbols are marked ".globl" during
their declaration, but their later mention uses __attribute__((weak)),
so it's better to only use ".weak" during the declaration so that the
symbol's class does not change.
No need to backport this unless someone reports build issues.
There's an explanation of the problem, when it's encountered, and the
reasoning behind the proposed solution. The follwing day the CI broke
on MacOS, I could restart from the elements above to try to design
another solution that still follows the same spirit (and in the worst
case it could possibly have been accepted to just revert and keep that
warning when debugging):
commit fb1b6f5bc0e8eac116e2cafe8716a7f16d95b58e
Author: Willy Tarreau
Date: Thu Apr 14 16:57:12 2022 +0200
BUILD: compiler: use a more portable set of asm(".weak") statements
The two recent patches b12966af1 ("BUILD: debug: mark the
__start_mem_stats/__stop_mem_stats symbols as weak") and 2a06e248f
("BUILD: initcall: mark the __start_i_* symbols as weak, not global")
aimed at fixing a build warning and resulted in a build breakage on
MacOS which doesn't have a ".weak" asm statement.
We've already had MacOS-specific asm() statements for section names, so
this patch continues on this trend by moving HA_GLOBL() to compiler.h
and using ".globl" on MacOS since apparently nobody complains there.
It is debatable whether to expose this only when !USE_OBSOLETE_LINKER
or all the time, but since these are just macroes it's no big deal to
let them be available when needed and let the caller decide on the
build conditions.
If any of the patches above is backported, this one will need to as
well.
The new choice was explained again. Finally,
