---------- Forwarded message ---------
From: David CARLIER <devne...@gmail.com>
Date: Wed, 24 Apr 2024 at 07:56
Subject: Re: [PATCH] MEDIUM: shctx naming shared memory context
To: Willy Tarreau <w...@1wt.eu>
Here a new version.
Cheers.
On Wed, 24 Apr 2024 at 07:45, Willy Tarreau <w...@1wt.eu> wrote:
> Hi David,
>
> On Sat, Apr 20, 2024 at 07:33:16AM +0100, David CARLIER wrote:
> > From d49d9d5966caead320f33f789578cb69f2aa3787 Mon Sep 17 00:00:00 2001
> > From: David Carlier <devne...@gmail.com>
> > Date: Sat, 20 Apr 2024 07:18:48 +0100
> > Subject: [PATCH] MEDIUM: shctx: Naming shared memory context
> >
> > From Linux 5.17, anonymous regions can be name via prctl/PR_SET_VMA
> > so caches can be identified when looking at HAProxy process memory
> > mapping.
>
> That's pretty cool, I wasn't aware of this feature, that's really
> interesting!
>
> However I disagree with this point:
>
> > +#if defined(USE_PRCTL) && defined(PR_SET_VMA)
> > + {
> > + /**
> > + * From Linux 5.17 (and if the `CONFIG_ANON_VMA_NAME`
> kernel config is set)`,
> > + * anonymous regions can be named.
> > + *
> > + * The naming can take up to 79 characters, accepting
> valid ASCII values
> > + * except [, ], \, $ and '.
> > + * As a result, when looking for /proc/<pid>/maps, we can
> see the anonymous range
> > + * as follow :
> > + * `7364c4fff000-736508000000 rw-s 00000000 00:01 3540
> [anon_shmem:HAProxy globalCache]`
> > + */
> > + int rn;
> > + char fullname[80];
> > +
> > + rn = snprintf(fullname, sizeof(fullname), "HAProxy %s",
> name);
> > + if (rn < 0 || prctl(PR_SET_VMA, PR_SET_VMA_ANON_NAME,
> (uintptr_t)shctx,
> > + totalsize, (uintptr_t)fullname) != 0) {
> > + munmap(shctx, totalsize);
> > + shctx = NULL;
> > + ret = SHCTX_E_ALLOC_CACHE;
> > + goto err;
> > + }
> > + }
> > +#endif
>
> You're making the mapping fail on any kernel that does not support the
> feature (hence apparently anything < 5.17 according to your description).
> IMHO we should just silently fall back to the default behavior, i.e. we
> couldn't assign the name, fine, we continue to run without a name, thus
> something like this:
>
> rn = snprintf(fullname, sizeof(fullname), "HAProxy %s", name);
> if (rn >= 0)
> prctl(PR_SET_VMA, PR_SET_VMA_ANON_NAME, (uintptr_t)shctx,
> totalsize, (uintptr_t)fullname);
>
> Could you please adjust it and verify that it's still OK for you ?
> Likewise I can check it on a 5.15 here.
>
> Thank you!
> Willy
>
From 72542db6b65994f1755d161fad04828f435f7134 Mon Sep 17 00:00:00 2001
From: David Carlier <devnexen@gmail.com>
Date: Sat, 20 Apr 2024 07:18:48 +0100
Subject: [PATCH] MEDIUM: shctx: Naming shared memory context
From Linux 5.17, anonymous regions can be name via prctl/PR_SET_VMA
so caches can be identified when looking at HAProxy process memory
mapping.
The most possible error is lack of kernel support, as a result
we ignore it, if the naming fails the mapping of memory context
ought to still occur.
---
include/haproxy/shctx.h | 2 +-
src/cache.c | 2 +-
src/shctx.c | 34 +++++++++++++++++++++++++++++++---
src/ssl_sock.c | 2 +-
4 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/include/haproxy/shctx.h b/include/haproxy/shctx.h
index a57cf1567..01bb09d32 100644
--- a/include/haproxy/shctx.h
+++ b/include/haproxy/shctx.h
@@ -21,7 +21,7 @@
int shctx_init(struct shared_context **orig_shctx,
int maxblocks, int blocksize, unsigned int maxobjsz,
- int extra);
+ int extra, __maybe_unused const char *name);
struct shared_block *shctx_row_reserve_hot(struct shared_context *shctx,
struct shared_block *last, int data_len);
void shctx_row_detach(struct shared_context *shctx, struct shared_block *first);
diff --git a/src/cache.c b/src/cache.c
index 4a9992a19..25f20a782 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -2471,7 +2471,7 @@ int post_check_cache()
list_for_each_entry_safe(cache_config, back, &caches_config, list) {
ret_shctx = shctx_init(&shctx, cache_config->maxblocks, CACHE_BLOCKSIZE,
- cache_config->maxobjsz, sizeof(struct cache));
+ cache_config->maxobjsz, sizeof(struct cache), cache_config->id);
if (ret_shctx <= 0) {
if (ret_shctx == SHCTX_E_INIT_LOCK)
diff --git a/src/shctx.c b/src/shctx.c
index be5905392..6c7ad172d 100644
--- a/src/shctx.c
+++ b/src/shctx.c
@@ -16,6 +16,9 @@
#include <import/ebmbtree.h>
#include <haproxy/list.h>
#include <haproxy/shctx.h>
+#if defined(USE_PRCTL)
+#include <sys/prctl.h>
+#endif
/*
* Reserve a new row if <first> is null, put it in the hotlist, set the refcount to 1
@@ -269,13 +272,14 @@ int shctx_row_data_get(struct shared_context *shctx, struct shared_block *first,
* and 0 if cache is already allocated.
*/
int shctx_init(struct shared_context **orig_shctx, int maxblocks, int blocksize,
- unsigned int maxobjsz, int extra)
+ unsigned int maxobjsz, int extra, const char *name)
{
int i;
struct shared_context *shctx;
int ret;
void *cur;
int maptype = MAP_SHARED;
+ size_t totalsize = sizeof(struct shared_context) + extra + (maxblocks * (sizeof(struct shared_block) + blocksize));
if (maxblocks <= 0)
return 0;
@@ -284,14 +288,38 @@ int shctx_init(struct shared_context **orig_shctx, int maxblocks, int blocksize,
blocksize = (blocksize + sizeof(void *) - 1) & -sizeof(void *);
extra = (extra + sizeof(void *) - 1) & -sizeof(void *);
- shctx = (struct shared_context *)mmap(NULL, sizeof(struct shared_context) + extra + (maxblocks * (sizeof(struct shared_block) + blocksize)),
- PROT_READ | PROT_WRITE, maptype | MAP_ANON, -1, 0);
+ shctx = (struct shared_context *)mmap(NULL, totalsize, PROT_READ | PROT_WRITE, maptype | MAP_ANON, -1, 0);
if (!shctx || shctx == MAP_FAILED) {
shctx = NULL;
ret = SHCTX_E_ALLOC_CACHE;
goto err;
}
+#if defined(USE_PRCTL) && defined(PR_SET_VMA)
+ {
+ /**
+ * From Linux 5.17 (and if the `CONFIG_ANON_VMA_NAME` kernel config is set)`,
+ * anonymous regions can be named.
+ * We intentionally ignore errors as it should not jeopardize the memory context
+ * mapping whatsoever (e.g. older kernels).
+ *
+ * The naming can take up to 79 characters, accepting valid ASCII values
+ * except [, ], \, $ and '.
+ * As a result, when looking for /proc/<pid>/maps, we can see the anonymous range
+ * as follow :
+ * `7364c4fff000-736508000000 rw-s 00000000 00:01 3540 [anon_shmem:HAProxy globalCache]`
+ */
+ int rn;
+ char fullname[80];
+
+ rn = snprintf(fullname, sizeof(fullname), "HAProxy %s", name);
+ if (rn >= 0) {
+ (void)prctl(PR_SET_VMA, PR_SET_VMA_ANON_NAME, (uintptr_t)shctx,
+ totalsize, (uintptr_t)fullname);
+ }
+ }
+#endif
+
shctx->nbav = 0;
LIST_INIT(&shctx->avail);
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 9df175d93..31885efe6 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -5351,7 +5351,7 @@ int ssl_sock_prepare_bind_conf(struct bind_conf *bind_conf)
if (!ssl_shctx && global.tune.sslcachesize) {
alloc_ctx = shctx_init(&ssl_shctx, global.tune.sslcachesize,
sizeof(struct sh_ssl_sess_hdr) + SHSESS_BLOCK_MIN_SIZE, -1,
- sizeof(*sh_ssl_sess_tree));
+ sizeof(*sh_ssl_sess_tree), "ssl cache");
if (alloc_ctx <= 0) {
if (alloc_ctx == SHCTX_E_INIT_LOCK)
ha_alert("Unable to initialize the lock for the shared SSL session cache. You can retry using the global statement 'tune.ssl.force-private-cache' but it could increase CPU usage due to renegotiations if nbproc > 1.\n");
--
2.43.0
