Patchwork [4,of,4] chg: change server's process title

login
register
mail settings
Submitter Jun Wu
Date Aug. 10, 2016, 5:30 p.m.
Message ID <e7c880aa65ef1cdf00ff.1470850200@x1c>
Download mbox | patch
Permalink /patch/16243/
State Changes Requested
Headers show

Comments

Jun Wu - Aug. 10, 2016, 5:30 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1470849617 -3600
#      Wed Aug 10 18:20:17 2016 +0100
# Node ID e7c880aa65ef1cdf00ffe0db27d4365294cb6a62
# Parent  7472b38c08ca30af74668f41056afbd065887c04
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r e7c880aa65ef
chg: change server's process title

This patch uses the newly introduced "setprocname" interface to update the
process title server-side, to make it easier to tell what a worker is
actually doing.

The title is short, "chg:$PID", to fit in prctl's 15-char limit. The new
title can be observed using "ps -AO comm" under Linux, and is directly
observable using "ps -A" under FreeBSD.
Yuya Nishihara - Aug. 11, 2016, 2:28 p.m.
On Wed, 10 Aug 2016 18:30:00 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1470849617 -3600
> #      Wed Aug 10 18:20:17 2016 +0100
> # Node ID e7c880aa65ef1cdf00ffe0db27d4365294cb6a62
> # Parent  7472b38c08ca30af74668f41056afbd065887c04
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r e7c880aa65ef
> chg: change server's process title
> 
> This patch uses the newly introduced "setprocname" interface to update the
> process title server-side, to make it easier to tell what a worker is
> actually doing.
> 
> The title is short, "chg:$PID", to fit in prctl's 15-char limit. The new
> title can be observed using "ps -AO comm" under Linux, and is directly
> observable using "ps -A" under FreeBSD.

> +static void updateprocname(hgclient_t *hgc)
> +{
> +	/* prctl(PR_SET_NAME) has a 15-char limit. be as short as possible */
> +	size_t n = (size_t)snprintf(hgc->ctx.data, hgc->ctx.maxdatasize,
> +			"chg:%d", (int)getpid());
> +	hgc->ctx.datasize = n;
> +	writeblockrequest(hgc, "setprocname");
> +}

If it's displayed as if argv were {"chg:%d"} on FreeBSD, it would be confusing
that both client and server are named as "chg".

I'm not enthusiastic about this series because it requires new C functions
and command-server command just for ease of debugging, different platform
behaviors (I guess), no OSX support.
Gregory Szorc - Aug. 11, 2016, 4 p.m.
> On Aug 11, 2016, at 07:28, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Wed, 10 Aug 2016 18:30:00 +0100, Jun Wu wrote:
>> # HG changeset patch
>> # User Jun Wu <quark@fb.com>
>> # Date 1470849617 -3600
>> #      Wed Aug 10 18:20:17 2016 +0100
>> # Node ID e7c880aa65ef1cdf00ffe0db27d4365294cb6a62
>> # Parent  7472b38c08ca30af74668f41056afbd065887c04
>> # Available At https://bitbucket.org/quark-zju/hg-draft
>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r e7c880aa65ef
>> chg: change server's process title
>> 
>> This patch uses the newly introduced "setprocname" interface to update the
>> process title server-side, to make it easier to tell what a worker is
>> actually doing.
>> 
>> The title is short, "chg:$PID", to fit in prctl's 15-char limit. The new
>> title can be observed using "ps -AO comm" under Linux, and is directly
>> observable using "ps -A" under FreeBSD.
> 
>> +static void updateprocname(hgclient_t *hgc)
>> +{
>> +    /* prctl(PR_SET_NAME) has a 15-char limit. be as short as possible */
>> +    size_t n = (size_t)snprintf(hgc->ctx.data, hgc->ctx.maxdatasize,
>> +            "chg:%d", (int)getpid());
>> +    hgc->ctx.datasize = n;
>> +    writeblockrequest(hgc, "setprocname");
>> +}
> 
> If it's displayed as if argv were {"chg:%d"} on FreeBSD, it would be confusing
> that both client and server are named as "chg".
> 
> I'm not enthusiastic about this series because it requires new C functions
> and command-server command just for ease of debugging, different platform
> behaviors (I guess), no OSX support.

And I've long wanted to abuse process title setting in hgweb servers - especially when they are configured to serve multiple repos. It's difficult to see which repo is consuming CPU *right now* without it.
Jun Wu - Aug. 11, 2016, 4:24 p.m.
Excerpts from Yuya Nishihara's message of 2016-08-11 23:28:20 +0900:
> On Wed, 10 Aug 2016 18:30:00 +0100, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1470849617 -3600
> > #      Wed Aug 10 18:20:17 2016 +0100
> > # Node ID e7c880aa65ef1cdf00ffe0db27d4365294cb6a62
> > # Parent  7472b38c08ca30af74668f41056afbd065887c04
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r e7c880aa65ef
> > chg: change server's process title
> > 
> > This patch uses the newly introduced "setprocname" interface to update the
> > process title server-side, to make it easier to tell what a worker is
> > actually doing.
> > 
> > The title is short, "chg:$PID", to fit in prctl's 15-char limit. The new
> > title can be observed using "ps -AO comm" under Linux, and is directly
> > observable using "ps -A" under FreeBSD.
> 
> > +static void updateprocname(hgclient_t *hgc)
> > +{
> > +    /* prctl(PR_SET_NAME) has a 15-char limit. be as short as possible */
> > +    size_t n = (size_t)snprintf(hgc->ctx.data, hgc->ctx.maxdatasize,
> > +            "chg:%d", (int)getpid());
> > +    hgc->ctx.datasize = n;
> > +    writeblockrequest(hgc, "setprocname");
> > +}
> 
> If it's displayed as if argv were {"chg:%d"} on FreeBSD, it would be confusing
> that both client and server are named as "chg".

":" can be used to distinguish servers from clients. I'm open to ideas about
better format string but pid can be as long as 10 chars, or at least 7 chars
so we have little space left for Linux.

> I'm not enthusiastic about this series because it requires new C functions
> and command-server command just for ease of debugging, different platform
> behaviors (I guess), no OSX support.

Unfortunately I don't see a better way to do this. Some people from other
teams want this feature badly and I think it's a worthy one. From time to
time, the worker fails to write its pid to blackbox and we have to check
process startup time to find the matching chg process - not a pleasant
experience.

If we really want OS X support, it is probably doable like what node.js
does [1], although the code and comment scares me a bit.

[1]: https://github.com/openwebos/nodejs/blob/master/src/platform_darwin_proctitle.cc
Yuya Nishihara - Aug. 11, 2016, 4:55 p.m.
On Thu, 11 Aug 2016 17:24:36 +0100, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-08-11 23:28:20 +0900:
> > On Wed, 10 Aug 2016 18:30:00 +0100, Jun Wu wrote:
> > > # HG changeset patch
> > > # User Jun Wu <quark@fb.com>
> > > # Date 1470849617 -3600
> > > #      Wed Aug 10 18:20:17 2016 +0100
> > > # Node ID e7c880aa65ef1cdf00ffe0db27d4365294cb6a62
> > > # Parent  7472b38c08ca30af74668f41056afbd065887c04
> > > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r e7c880aa65ef
> > > chg: change server's process title
> > > 
> > > This patch uses the newly introduced "setprocname" interface to update the
> > > process title server-side, to make it easier to tell what a worker is
> > > actually doing.
> > > 
> > > The title is short, "chg:$PID", to fit in prctl's 15-char limit. The new
> > > title can be observed using "ps -AO comm" under Linux, and is directly
> > > observable using "ps -A" under FreeBSD.
> > 
> > > +static void updateprocname(hgclient_t *hgc)
> > > +{
> > > +    /* prctl(PR_SET_NAME) has a 15-char limit. be as short as possible */
> > > +    size_t n = (size_t)snprintf(hgc->ctx.data, hgc->ctx.maxdatasize,
> > > +            "chg:%d", (int)getpid());
> > > +    hgc->ctx.datasize = n;
> > > +    writeblockrequest(hgc, "setprocname");
> > > +}
> > 
> > If it's displayed as if argv were {"chg:%d"} on FreeBSD, it would be confusing
> > that both client and server are named as "chg".
> 
> ":" can be used to distinguish servers from clients. I'm open to ideas about
> better format string but pid can be as long as 10 chars, or at least 7 chars
> so we have little space left for Linux.
> 
> > I'm not enthusiastic about this series because it requires new C functions
> > and command-server command just for ease of debugging, different platform
> > behaviors (I guess), no OSX support.
> 
> Unfortunately I don't see a better way to do this. Some people from other
> teams want this feature badly and I think it's a worthy one. From time to
> time, the worker fails to write its pid to blackbox and we have to check
> process startup time to find the matching chg process - not a pleasant
> experience.

Maybe we'll need setproctitle() equivalent for Linux which will update argv?
16-char limitation of prctl() would come from pthread API. If we want to
extend the (ab)use of process title to hgweb, PR_SET_NAME won't be useful.
It won't be displayed in normal ps output.

I found some examples in CRuby (MRI) and PostgreSQL code, though I don't
know if OSX is supported.

> If we really want OS X support, it is probably doable like what node.js
> does [1], although the code and comment scares me a bit.
> 
> [1]: https://github.com/openwebos/nodejs/blob/master/src/platform_darwin_proctitle.cc
Jun Wu - Aug. 11, 2016, 5:28 p.m.
Excerpts from Yuya Nishihara's message of 2016-08-12 01:55:44 +0900:
> Maybe we'll need setproctitle() equivalent for Linux which will update argv?

That would be probably rewriting argv[0], afaik. I think it is impossible
to get the "argv" pointer in a reasonable way.

> 16-char limitation of prctl() would come from pthread API. If we want to
> extend the (ab)use of process title to hgweb, PR_SET_NAME won't be useful.
> It won't be displayed in normal ps output.

It's still useful with '-O comm', and atop will record it. prctl is lower
level than pthread. Sadly 16 is hardcoded in Linux.

> I found some examples in CRuby (MRI) and PostgreSQL code, though I don't
> know if OSX is supported.

I was aware "ARGV[0] = 'foo'" works and "Process.setproctitle" was
introduced in some new version of Ruby. I guess they just rewrite argv[0].

What I can confirm now is neither of them works with Ruby 2.0.0 on OS X -
the latter is not implemented.

> > If we really want OS X support, it is probably doable like what node.js
> > does [1], although the code and comment scares me a bit.
> > 
> > [1]: https://github.com/openwebos/nodejs/blob/master/src/platform_darwin_proctitle.cc
Yuya Nishihara - Aug. 12, 2016, 12:26 a.m.
On Thu, 11 Aug 2016 18:28:09 +0100, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-08-12 01:55:44 +0900:
> > Maybe we'll need setproctitle() equivalent for Linux which will update argv?
> 
> That would be probably rewriting argv[0], afaik. I think it is impossible
> to get the "argv" pointer in a reasonable way.

Er, Py_GetArgcArgv() ? No idea if we can reliably use this hidden API.

https://hg.python.org/cpython/file/v2.7.11/Modules/main.c#l691

> > 16-char limitation of prctl() would come from pthread API. If we want to
> > extend the (ab)use of process title to hgweb, PR_SET_NAME won't be useful.
> > It won't be displayed in normal ps output.
> 
> It's still useful with '-O comm', and atop will record it. prctl is lower
> level than pthread. Sadly 16 is hardcoded in Linux.

I assume '-O comm' would be less common for Linux sysadmins.
Jun Wu - Aug. 12, 2016, 10:18 a.m.
Excerpts from Yuya Nishihara's message of 2016-08-12 09:26:37 +0900:
> On Thu, 11 Aug 2016 18:28:09 +0100, Jun Wu wrote:
> > Excerpts from Yuya Nishihara's message of 2016-08-12 01:55:44 +0900:
> > > Maybe we'll need setproctitle() equivalent for Linux which will update argv?
> > 
> > That would be probably rewriting argv[0], afaik. I think it is impossible
> > to get the "argv" pointer in a reasonable way.
> 
> Er, Py_GetArgcArgv() ? No idea if we can reliably use this hidden API.
> 
> https://hg.python.org/cpython/file/v2.7.11/Modules/main.c#l691 

Nice find! I did see "static char **orig_argv;" but missed the comment
around it. I'll try the approach. The memory handling could be a bit tricky.

One reason I was using prctl is because it is well defined on Linux, with
the annoying 15-char limit. argv hack is not portable and may have
compatibility issues with other strange code. I will double check with
MRIRuby.

> > > 16-char limitation of prctl() would come from pthread API. If we want to
> > > extend the (ab)use of process title to hgweb, PR_SET_NAME won't be useful.
> > > It won't be displayed in normal ps output.
> > 
> > It's still useful with '-O comm', and atop will record it. prctl is lower
> > level than pthread. Sadly 16 is hardcoded in Linux.
> 
> I assume '-O comm' would be less common for Linux sysadmins.
Gregory Szorc - Aug. 12, 2016, 4:42 p.m.
> On Aug 11, 2016, at 09:55, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Thu, 11 Aug 2016 17:24:36 +0100, Jun Wu wrote:
>> Excerpts from Yuya Nishihara's message of 2016-08-11 23:28:20 +0900:
>>>> On Wed, 10 Aug 2016 18:30:00 +0100, Jun Wu wrote:
>>>> # HG changeset patch
>>>> # User Jun Wu <quark@fb.com>
>>>> # Date 1470849617 -3600
>>>> #      Wed Aug 10 18:20:17 2016 +0100
>>>> # Node ID e7c880aa65ef1cdf00ffe0db27d4365294cb6a62
>>>> # Parent  7472b38c08ca30af74668f41056afbd065887c04
>>>> # Available At https://bitbucket.org/quark-zju/hg-draft 
>>>> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r e7c880aa65ef
>>>> chg: change server's process title
>>>> 
>>>> This patch uses the newly introduced "setprocname" interface to update the
>>>> process title server-side, to make it easier to tell what a worker is
>>>> actually doing.
>>>> 
>>>> The title is short, "chg:$PID", to fit in prctl's 15-char limit. The new
>>>> title can be observed using "ps -AO comm" under Linux, and is directly
>>>> observable using "ps -A" under FreeBSD.
>>> 
>>>> +static void updateprocname(hgclient_t *hgc)
>>>> +{
>>>> +    /* prctl(PR_SET_NAME) has a 15-char limit. be as short as possible */
>>>> +    size_t n = (size_t)snprintf(hgc->ctx.data, hgc->ctx.maxdatasize,
>>>> +            "chg:%d", (int)getpid());
>>>> +    hgc->ctx.datasize = n;
>>>> +    writeblockrequest(hgc, "setprocname");
>>>> +}
>>> 
>>> If it's displayed as if argv were {"chg:%d"} on FreeBSD, it would be confusing
>>> that both client and server are named as "chg".
>> 
>> ":" can be used to distinguish servers from clients. I'm open to ideas about
>> better format string but pid can be as long as 10 chars, or at least 7 chars
>> so we have little space left for Linux.
>> 
>>> I'm not enthusiastic about this series because it requires new C functions
>>> and command-server command just for ease of debugging, different platform
>>> behaviors (I guess), no OSX support.
>> 
>> Unfortunately I don't see a better way to do this. Some people from other
>> teams want this feature badly and I think it's a worthy one. From time to
>> time, the worker fails to write its pid to blackbox and we have to check
>> process startup time to find the matching chg process - not a pleasant
>> experience.
> 
> Maybe we'll need setproctitle() equivalent for Linux which will update argv?
> 16-char limitation of prctl() would come from pthread API. If we want to
> extend the (ab)use of process title to hgweb, PR_SET_NAME won't be useful.
> It won't be displayed in normal ps output.
> 
> I found some examples in CRuby (MRI) and PostgreSQL code, though I don't
> know if OSX is supported.

Someone whom I trust for technical advice tells me the PostgreSQL code for this is worth emulating.

> 
>> If we really want OS X support, it is probably doable like what node.js
>> does [1], although the code and comment scares me a bit.
>> 
>> [1]: https://github.com/openwebos/nodejs/blob/master/src/platform_darwin_proctitle.cc

Patch

diff --git a/contrib/chg/hgclient.c b/contrib/chg/hgclient.c
--- a/contrib/chg/hgclient.c
+++ b/contrib/chg/hgclient.c
@@ -35,6 +35,7 @@  enum {
 	CAP_SETENV = 0x0800,
 	CAP_SETUMASK = 0x1000,
 	CAP_VALIDATE = 0x2000,
+	CAP_SETPROCNAME = 0x4000,
 };
 
 typedef struct {
@@ -51,6 +52,7 @@  static const cappair_t captable[] = {
 	{"setenv", CAP_SETENV},
 	{"setumask", CAP_SETUMASK},
 	{"validate", CAP_VALIDATE},
+	{"setprocname", CAP_SETPROCNAME},
 	{NULL, 0},  /* terminator */
 };
 
@@ -350,6 +352,15 @@  static void readhello(hgclient_t *hgc)
 	debugmsg("capflags=0x%04x, pid=%d", hgc->capflags, hgc->pid);
 }
 
+static void updateprocname(hgclient_t *hgc)
+{
+	/* prctl(PR_SET_NAME) has a 15-char limit. be as short as possible */
+	size_t n = (size_t)snprintf(hgc->ctx.data, hgc->ctx.maxdatasize,
+			"chg:%d", (int)getpid());
+	hgc->ctx.datasize = n;
+	writeblockrequest(hgc, "setprocname");
+}
+
 static void attachio(hgclient_t *hgc)
 {
 	debugmsg("request attachio");
@@ -445,6 +456,8 @@  hgclient_t *hgc_open(const char *socknam
 	readhello(hgc);
 	if (!(hgc->capflags & CAP_RUNCOMMAND))
 		abortmsg("insufficient capability: runcommand");
+	if (hgc->capflags & CAP_SETPROCNAME)
+		updateprocname(hgc);
 	if (hgc->capflags & CAP_ATTACHIO)
 		attachio(hgc);
 	if (hgc->capflags & CAP_CHDIR)