From owner-svn-src-all@freebsd.org Thu Dec 8 04:52:57 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 80AE2C6C813; Thu, 8 Dec 2016 04:52:57 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id 063298A8; Thu, 8 Dec 2016 04:52:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id DE45BD61A2D; Thu, 8 Dec 2016 15:34:38 +1100 (AEDT) Date: Thu, 8 Dec 2016 15:34:38 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eric van Gyzen cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r309676 - in head: bin/ps lib/libkvm sys/compat/freebsd32 sys/kern sys/sys usr.bin/procstat usr.bin/top In-Reply-To: <201612071504.uB7F4MCi020382@repo.freebsd.org> Message-ID: <20161208130730.O1089@besplex.bde.org> References: <201612071504.uB7F4MCi020382@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=BKLDlBYG c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=v201P0twmFuHjgEfyYgA:9 a=KvgGgRznTEP-dP89:21 a=RZVXN80Jd8c52keY:21 a=ncrJ1ZNykZ1J1srM:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Dec 2016 04:52:57 -0000 On Wed, 7 Dec 2016, Eric van Gyzen wrote: > Log: > Export the whole thread name in kinfo_proc > > kinfo_proc::ki_tdname is three characters shorter than > thread::td_name. Add a ki_moretdname field for these three > extra characters. Add the new field to kinfo_proc32, as well. > Update all in-tree consumers to read the new field and assemble > the full name, except for lldb's HostThreadFreeBSD.cpp, which > I will handle separately. Bump __FreeBSD_version. tdname stuff was very badly implemented. It abuses the ki_ocomm field for ki_tdname. ki_ocomm was reserved for the old command name to support old applications. It should have been populated with an intelligently abbreviated copy of ki_comm, so that old applications which don't know about the ki_comm renaming see a useful name. It was actually populated by blindly truncating p_comm. This was good enough in most cases. Then when thread support was added, ki_ocomm was broken copying td_name to it. For the non-threaded case, this makes no difference, but for the threaded case this breaks ki_ocomm for old applications and it doesn't even work right for the threaded case since ki_ocomm is too short. This is the problem that you are partly fixing now. ki_ocomm should have remained as the truncated ki_comm. ki_comm should have been modified to contain an intelligently abbreviated copy of the command and thread names, so that not so old applications see any extra info in the thread name. A new field was needed for the thread name, and perhaps for the previous (non-mixed) command name, so that applications can see the raw names p_comm and td_name if the understand threads. Renaming ki_comm was rather gratuitous. It was only expanded from 16+1 to 19+1 bytes. This was done mainly because 16+1 gives either misalgnment or wastes space. The latter occurred for p_comm[] in struct proc. So we expanded to 19+1 but no more to get a slightly longer name at no cost in struct proc. But there was a large cost for using this. ac_comm[] in struct acct still has size 16+0 (it is not NUL-terminated). And the expansion didn't just work in user.h since the ki_comm[16+1] was packed there. The expansion was done cleanly by keeping renaming the old field but renaming it to ki_ocomm[16+1], and adding a new field ki_comm[19+1]. Then abusing ki_ocomm made a mess. ki_ocomm and OCOMMLEN are under a bogus BURN_BRIDGES ifdef in user.h. This doesn't even break the ABI. It only breaks the API, but that is less important and can be done in the head branch without any ifdefs (just after you update all uses of the API in src). The ABI was broken by abusing ki_ocomm. There is also much ugliness and loss of info by combining ki_comm and ki_tdname unintelligently and and then truncating. The combined length of 19+16 (now +3 more) is too wide for most displays. The unintelligences also adds excessive markup like '[]{}:' characters, giving lengths of 40+. Then blind truncation tends to lost the most useful info at the end. Your 'more' field instead of renaming again not as clean or wasteful of space as renaming again, but it is less invasive. > Modified: head/bin/ps/print.c > ============================================================================== > --- head/bin/ps/print.c Wed Dec 7 14:35:05 2016 (r309675) > +++ head/bin/ps/print.c Wed Dec 7 15:04:22 2016 (r309676) > @@ -120,11 +120,12 @@ command(KINFO *k, VARENT *ve) > if (cflag) { > /* If it is the last field, then don't pad */ > if (STAILQ_NEXT(ve, next_ve) == NULL) { > - asprintf(&str, "%s%s%s%s", > + asprintf(&str, "%s%s%s%s%s", > k->ki_d.prefix ? k->ki_d.prefix : "", > k->ki_p->ki_comm, > (showthreads && k->ki_p->ki_numthreads > 1) ? "/" : "", > - (showthreads && k->ki_p->ki_numthreads > 1) ? k->ki_p->ki_tdname : ""); > + (showthreads && k->ki_p->ki_numthreads > 1) ? k->ki_p->ki_tdname : "", > + (showthreads && k->ki_p->ki_numthreads > 1) ? k->ki_p->ki_moretdname : ""); This expands the style bugs by copying a too-long line and expanding it further. With intelligent combination, you couldn't do it in a big printf() arg list. FreeBSD cluster machines seem to have stopped showing system processes (even idle) in ps and top, so I can't easily see many bad messes from unintelligent combination or debug things related to system load. Locally, I get for top -SH: PID USERNAME PRI NICE SIZE RES STATE C TIME WCPU COMMAND 10 root 155 ki31 0K 64K CPU3 3 1:59 100.27% idle{idle: cpu3} 10 root 155 ki31 0K 64K CPU5 5 1:59 100.27% idle{idle: cpu5} 10 root 155 ki31 0K 64K CPU6 6 1:59 100.27% idle{idle: cpu6} 10 root 155 ki31 0K 64K CPU7 7 1:59 100.27% idle{idle: cpu7} 10 root 155 ki31 0K 64K CPU2 2 1:59 100.27% idle{idle: cpu2} 10 root 155 ki31 0K 64K CPU1 1 1:58 100.27% idle{idle: cpu1} 10 root 155 ki31 0K 64K RUN 0 1:58 100.27% idle{idle: cpu0} 10 root 155 ki31 0K 64K CPU4 4 1:58 100.27% idle{idle: cpu4} 0 root -16 - 0K 160K swapin 5 0:29 0.00% kernel{swapper} 13 root -68 - 0K 120K - 0 0:00 0.00% usb{usbus2} 13 root -68 - 0K 120K - 5 0:00 0.00% usb{usbus1} 0 root 8 - 0K 160K - 0 0:00 0.00% kernel{thread ta 1 root 8 0 732K 372K wait 4 0:00 0.00% init 11 root -60 - 0K 168K WAIT 0 0:00 0.00% intr{swi4: clock 11 root -88 - 0K 168K WAIT 4 0:00 0.00% intr{irq265: ahc 13 root -68 - 0K 120K - 0 0:00 0.00% usb{usbus0} 12 root -8 - 0K 24K - 0 0:00 0.00% geom{g_event} 678 bde 8 0 6344K 6168K wait 6 0:00 0.00% bash This suffers some truncation, but freefall is much worse: some undisplayed username is wider, so 5 columns are wasted for USERNAME and only 11 are left. for COMMAND. The above shows the common lossage that the first part of ki_tdname is the same as the whole ki_comm. ki_tdname needs to be longer to hold this, but is shorter. Unintelligent combination duplicates ki_comm and then truncates info. The other names are less consistently bad. "usb" is duplicated, and "intr" is duplicated as "irq". Other names mostly are just too long. ki_tdname is sometimes a description, not a name. ki_comm doesn't claim to be a name, but more usually is. It takes -a to get args for top. top -a and ps l suffer from to much truncation of COMMAND to actually show many args. > } else > str = strdup(k->ki_p->ki_comm); > > @@ -172,14 +173,16 @@ ucomm(KINFO *k, VARENT *ve) > char *str; > > if (STAILQ_NEXT(ve, next_ve) == NULL) { /* last field, don't pad */ > - asprintf(&str, "%s%s%s%s", > + asprintf(&str, "%s%s%s%s%s", > k->ki_d.prefix ? k->ki_d.prefix : "", > k->ki_p->ki_comm, > (showthreads && k->ki_p->ki_numthreads > 1) ? "/" : "", > - (showthreads && k->ki_p->ki_numthreads > 1) ? k->ki_p->ki_tdname : ""); > + (showthreads && k->ki_p->ki_numthreads > 1) ? k->ki_p->ki_tdname : "", > + (showthreads && k->ki_p->ki_numthreads > 1) ? k->ki_p->ki_moretdname : ""); As above. > } else { > if (showthreads && k->ki_p->ki_numthreads > 1) > - asprintf(&str, "%s/%s", k->ki_p->ki_comm, k->ki_p->ki_tdname); > + asprintf(&str, "%s/%s%s", k->ki_p->ki_comm, > + k->ki_p->ki_tdname, k->ki_p->ki_moretdname); > else > str = strdup(k->ki_p->ki_comm); > } Here you fixed the formatting style bug, but it might be considered a style bug to use strdup() instead of asprintf() for the simple case. style(9) says to always use fprintf() and not fputs()...or whatever, and we can do the same things for asprintf() to get uniform logic for the allocation. The printf() part is just not so uniform when its arg list depends on (showthreads && k->ki_p->ki_numthreads > 1). > @@ -192,7 +195,8 @@ tdnam(KINFO *k, VARENT *ve __unused) > char *str; > > if (showthreads && k->ki_p->ki_numthreads > 1) > - str = strdup(k->ki_p->ki_tdname); > + asprintf(&str, "%s%s", k->ki_p->ki_tdname, > + k->ki_p->ki_moretdname); > else > str = strdup(" "); Here always using asprintf() is clearly better because it avoids having to count the spaces in the literal string. > Modified: head/lib/libkvm/kvm_proc.c > ============================================================================== > --- head/lib/libkvm/kvm_proc.c Wed Dec 7 14:35:05 2016 (r309675) > +++ head/lib/libkvm/kvm_proc.c Wed Dec 7 15:04:22 2016 (r309676) > @@ -426,8 +426,6 @@ nopgrp: > kp->ki_pri.pri_native = mtd.td_base_pri; > kp->ki_lastcpu = mtd.td_lastcpu; > kp->ki_wchan = mtd.td_wchan; > - if (mtd.td_name[0] != 0) > - strlcpy(kp->ki_tdname, mtd.td_name, MAXCOMLEN); This seems to have been dead code. > kp->ki_oncpu = mtd.td_oncpu; > if (mtd.td_name[0] != '\0') > strlcpy(kp->ki_tdname, mtd.td_name, sizeof(kp->ki_tdname)); > In my version, most string lengths are not #defined and/or not exported in user.h, so as to force use of sizeof() like here. > Modified: head/sys/compat/freebsd32/freebsd32.h > ============================================================================== > --- head/sys/compat/freebsd32/freebsd32.h Wed Dec 7 14:35:05 2016 (r309675) > +++ head/sys/compat/freebsd32/freebsd32.h Wed Dec 7 15:04:22 2016 (r309676) > @@ -315,7 +315,8 @@ struct kinfo_proc32 { > char ki_comm[COMMLEN+1]; > char ki_emul[KI_EMULNAMELEN+1]; > char ki_loginclass[LOGINCLASSLEN+1]; > - char ki_sparestrings[50]; > + char ki_moretdname[MAXCOMLEN-TDNAMLEN+1]; > + char ki_sparestrings[46]; It's inconsistent to use #define'd values to calculate 3+1 for the size of the new field, then hard code this 3+1 in the adjustment of 50 to 46. I prefer to hard-code all the array sizes and then use sizeof() to recover them. I only do this in usee.h, not here. > int ki_spareints[KI_NSPARE_INT]; The #define's for the spare counts are more actively harmful. You needed to not have one for the char spares to make the above magic adjustment of 4 as easy as possible. > int ki_oncpu; > int ki_lastcpu; > > Modified: head/sys/kern/kern_proc.c > ============================================================================== > --- head/sys/kern/kern_proc.c Wed Dec 7 14:35:05 2016 (r309675) > +++ head/sys/kern/kern_proc.c Wed Dec 7 15:04:22 2016 (r309676) > @@ -1020,7 +1020,14 @@ fill_kinfo_thread(struct thread *td, str > strlcpy(kp->ki_wmesg, td->td_wmesg, sizeof(kp->ki_wmesg)); > else > bzero(kp->ki_wmesg, sizeof(kp->ki_wmesg)); > - strlcpy(kp->ki_tdname, td->td_name, sizeof(kp->ki_tdname)); > + if (strlcpy(kp->ki_tdname, td->td_name, sizeof(kp->ki_tdname)) >= > + sizeof(kp->ki_tdname)) { > + strlcpy(kp->ki_moretdname, > + td->td_name + sizeof(kp->ki_tdname) - 1, > + sizeof(kp->ki_moretdname)); > + } else { > + bzero(kp->ki_moretdname, sizeof(kp->ki_moretdname)); > + } I think this bzero() and the one above for wmesg and a couple of others are bogus. There should have been a bzero() of the whole struct to fill any padding, and that fills all zero fields too. > Modified: head/sys/sys/user.h > ============================================================================== > --- head/sys/sys/user.h Wed Dec 7 14:35:05 2016 (r309675) > +++ head/sys/sys/user.h Wed Dec 7 15:04:22 2016 (r309676) > @@ -180,12 +180,13 @@ struct kinfo_proc { > char ki_comm[COMMLEN+1]; /* command name */ > char ki_emul[KI_EMULNAMELEN+1]; /* emulation name */ > char ki_loginclass[LOGINCLASSLEN+1]; /* login class */ > + char ki_moretdname[MAXCOMLEN-TDNAMLEN+1]; /* more thread name */ > /* > * When adding new variables, take space for char-strings from the > * front of ki_sparestrings, and ints from the end of ki_spareints. > * That way the spare room from both arrays will remain contiguous. > */ > - char ki_sparestrings[50]; /* spare string space */ > + char ki_sparestrings[46]; /* spare string space */ > int ki_spareints[KI_NSPARE_INT]; /* spare room for growth */ > int ki_oncpu; /* Which cpu we are on */ > int ki_lastcpu; /* Last cpu we were on */ > The #define's for the sizes are actually sort of needed. We are (ab)using the MI values in the compat headers. The values should be, but aren't required to be the same, especially for the spares. They are just what they have to be to preserve the ABI, and the ABIs have the same number of spares for historical reasons. Only strings are sure to have the same ABI for individual fields. > Modified: head/usr.bin/procstat/procstat.c > ============================================================================== > --- head/usr.bin/procstat/procstat.c Wed Dec 7 14:35:05 2016 (r309675) > +++ head/usr.bin/procstat/procstat.c Wed Dec 7 15:04:22 2016 (r309676) > @@ -35,6 +35,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -126,6 +127,21 @@ kinfo_proc_sort(struct kinfo_proc *kipp, > qsort(kipp, count, sizeof(*kipp), kinfo_proc_compare); > } > > +const char * > +kinfo_proc_thread_name(const struct kinfo_proc *kipp) > +{ > + static char name[MAXCOMLEN+1]; Don't use the style bug of no spaces around binary operators when nearby code sdoesn't already have it. > + > + strlcpy(name, kipp->ki_tdname, sizeof(name)); > + strlcat(name, kipp->ki_moretdname, sizeof(name)); Should probably use 1 snprintf(). > ... > Modified: head/usr.bin/procstat/procstat_threads.c > ============================================================================== > --- head/usr.bin/procstat/procstat_threads.c Wed Dec 7 14:35:05 2016 (r309675) > +++ head/usr.bin/procstat/procstat_threads.c Wed Dec 7 15:04:22 2016 (r309676) > @@ -71,11 +71,10 @@ procstat_threads(struct procstat *procst > xo_open_container(threadid); > xo_emit("{dk:process_id/%5d/%d} ", kipp->ki_pid); > xo_emit("{:thread_id/%6d/%d} ", kipp->ki_tid); > - xo_emit("{d:command/%-16s/%s} ", strlen(kipp->ki_comm) ? > + xo_emit("{d:command/%-19s/%s} ", strlen(kipp->ki_comm) ? > kipp->ki_comm : "-"); > - xo_emit("{:thread_name/%-16s/%s} ", (strlen(kipp->ki_tdname) && > - (strcmp(kipp->ki_comm, kipp->ki_tdname) != 0)) ? > - kipp->ki_tdname : "-"); > + xo_emit("{:thread_name/%-19s/%s} ", > + kinfo_proc_thread_name(kipp)); > if (kipp->ki_oncpu != 255) > xo_emit("{:cpu/%3d/%d} ", kipp->ki_oncpu); > else if (kipp->ki_lastcpu != 255) > It's actually more invasive to make applications concatentate the names, but if you added a new field with the correct length you would have to change lots of hard-coded 16's for the old wrong size. Hard-coding these in printf formats is easiest and almost OK since the ABI prevents the size changing often. > Modified: head/usr.bin/top/machine.c > ============================================================================== > --- head/usr.bin/top/machine.c Wed Dec 7 14:35:05 2016 (r309675) > +++ head/usr.bin/top/machine.c Wed Dec 7 15:04:22 2016 (r309676) > @@ -991,8 +991,8 @@ format_next_process(caddr_t handle, char > if (!(flags & FMT_SHOWARGS)) { > if (ps.thread && pp->ki_flag & P_HADTHREADS && > pp->ki_tdname[0]) { > - snprintf(cmdbuf, cmdlen, "%s{%s}", pp->ki_comm, > - pp->ki_tdname); > + snprintf(cmdbuf, cmdlen, "%s{%s%s}", pp->ki_comm, > + pp->ki_tdname, pp->ki_moretdname); > } else { > snprintf(cmdbuf, cmdlen, "%s", pp->ki_comm); > } procstat has a little more intelligence to produce '-' when the thread name just duplicates the command name, but according to my top output it rarely does. I think it mainly produces '-' by finding a null thread name for the unthreaded case. procstat uses strcmp(). Comparing prefixes could do more. The above uses the null thread name test and other things to produce a different format instead of a thread name of '-'. It is not so easy to use a general function to prepare the names for special formatting. top doesn't have the hard-coded 16's and 19's in format strings. It would have just worked (except for bad combination) with a clean rename. Bruce