From nobody Sun Jul 2 19:52:46 2023 X-Original-To: freebsd-hackers@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4QvKTw5vJbz4hc63 for ; Sun, 2 Jul 2023 19:52:52 +0000 (UTC) (envelope-from cliftonr@volcano.org) Received: from omta38.uswest2.a.cloudfilter.net (omta38.uswest2.a.cloudfilter.net [35.89.44.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "Client", Issuer "CA" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 4QvKTv6N3Rz3PrF for ; Sun, 2 Jul 2023 19:52:51 +0000 (UTC) (envelope-from cliftonr@volcano.org) Authentication-Results: mx1.freebsd.org; dkim=none; spf=pass (mx1.freebsd.org: domain of cliftonr@volcano.org designates 35.89.44.37 as permitted sender) smtp.mailfrom=cliftonr@volcano.org; dmarc=none Received: from eig-obgw-5003a.ext.cloudfilter.net ([10.0.29.159]) by cmsmtp with ESMTP id FvkKqnzyRQFHRG37pqVh6d; Sun, 02 Jul 2023 19:52:49 +0000 Received: from gator4150.hostgator.com ([192.185.4.162]) by cmsmtp with ESMTPS id G37oqQBAtdcygG37pqMVRN; Sun, 02 Jul 2023 19:52:49 +0000 X-Authority-Analysis: v=2.4 cv=QbN1A+Xv c=1 sm=1 tr=0 ts=64a1d591 a=UsLNsgIQu5Rwd0y+KnVsSA==:117 a=wNkKhEd54xca6NfFpypmcg==:17 a=IkcTkHD0fZMA:10 a=ws7JD89P4LkA:10 a=ENOBcGDIfdYA:10 a=6I5d2MoRAAAA:8 a=GjEiR67sAAAA:8 a=Lueq0QKXb3i0uZKH0LoA:9 a=QEXdDO2ut3YA:10 a=IjZwj45LgO3ly-622nXo:22 a=B-DU53zI3E2b2plE8BPh:22 Received: from dhcp-72-234-209-134.hawaiiantel.net ([72.234.209.134]:53324 helo=[10.0.0.12]) by gator4150.hostgator.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1qG37o-000OGg-2P for freebsd-hackers@freebsd.org; Sun, 02 Jul 2023 14:52:48 -0500 Message-ID: <2ad2965e-ccc4-a202-2a58-0b2970aad925@volcano.org> Date: Sun, 2 Jul 2023 09:52:46 -1000 List-Id: Technical discussions relating to FreeBSD List-Archive: https://lists.freebsd.org/archives/freebsd-hackers List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-hackers@freebsd.org MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: top vs. array sorted_state's out of bounds indexing (and related issues) Content-Language: en-US To: freebsd-hackers@freebsd.org References: From: Clifton Royston In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - gator4150.hostgator.com X-AntiAbuse: Original Domain - freebsd.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - volcano.org X-BWhitelist: no X-Source-IP: 72.234.209.134 X-Source-L: No X-Exim-ID: 1qG37o-000OGg-2P X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: dhcp-72-234-209-134.hawaiiantel.net ([10.0.0.12]) [72.234.209.134]:53324 X-Source-Auth: cliftonr@volcano.org X-Email-Count: 2 X-Org: HG=hgshared;ORG=hostgator; X-Source-Cap: Y2xpZnRvbnI7Y2xpZnRvbnI7Z2F0b3I0MTUwLmhvc3RnYXRvci5jb20= X-Local-Domain: yes X-CMAE-Envelope: MS4xfIBXX05cy/+wW4zGjvCF7NUjf8vSXmoW1wusQrE3iYsmejrW3C837sPigknQnWWiGFjIjoIO0zkzNwbo/qBlp2Wp2WmHal2EDskYV6g1kGqAPunQSu9w u1tZkprx3icPjDBjXcY6JOH3Akw9Fm8iqnuzAnNlQOK46MeGwsQavthFEJZ9eutl+vZWNjDU9CYbLfsFusurVghrdYX78Zf1OAY= X-Spamd-Result: default: False [-3.44 / 15.00]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.99)[-0.995]; NEURAL_HAM_MEDIUM(-0.94)[-0.944]; R_SPF_ALLOW(-0.20)[+ip4:35.89.44.32/29]; RCVD_IN_DNSWL_LOW(-0.10)[35.89.44.37:from]; MIME_GOOD(-0.10)[text/plain]; RWL_MAILSPIKE_GOOD(-0.10)[35.89.44.37:from]; R_DKIM_NA(0.00)[]; ASN(0.00)[asn:16509, ipnet:35.80.0.0/12, country:US]; RCVD_TLS_LAST(0.00)[]; MIME_TRACE(0.00)[0:+]; MLMMJ_DEST(0.00)[freebsd-hackers@freebsd.org]; FROM_EQ_ENVFROM(0.00)[]; HAS_X_ANTIABUSE(0.00)[]; BLOCKLISTDE_FAIL(0.00)[72.234.209.134:server fail,192.185.4.162:server fail,35.89.44.37:server fail]; RCVD_COUNT_THREE(0.00)[4]; RCPT_COUNT_ONE(0.00)[1]; FROM_HAS_DN(0.00)[]; ARC_NA(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; TO_DN_NONE(0.00)[]; HAS_X_SOURCE(0.00)[]; DMARC_NA(0.00)[volcano.org]; RCVD_VIA_SMTP_AUTH(0.00)[] X-Rspamd-Queue-Id: 4QvKTv6N3Rz3PrF X-Spamd-Bar: --- X-ThisMailContainsUnwantedMimeParts: N On 6/19/2023 6:05 AM, Konstantin Belousov wrote: > On Sun, Jun 18, 2023 at 09:23:06PM -0700, Mark Millard wrote: >> usr.bin/top/machine.c (from main) has: >> >> /* >> . . . The process states are ordered as follows (from least >> * to most important): WAIT, zombie, sleep, stop, start, run. The >> * array declaration below maps a process state index into a number >> * that reflects this ordering. >> */ >> >> static int sorted_state[] = { >> 0, /* not used */ >> 3, /* sleep */ >> 1, /* ABANDONED (WAIT) */ >> 6, /* run */ >> 5, /* start */ >> 2, /* zombie */ >> 4 /* stop */ >> }; >> >> Note the elements are 0..6, so 7 elements. >> >> It is accessed via code like: >> >> sorted_state[(unsigned char)(b)->ki_stat] [...] >> /* >> * These were process status values (p_stat), now they are only used in >> * legacy conversion code. >> */ >> #define SIDL 1 /* Process being created by fork. */ >> #define SRUN 2 /* Currently runnable. */ >> #define SSLEEP 3 /* Sleeping on an address. */ >> #define SSTOP 4 /* Process debugging or suspension. */ >> #define SZOMB 5 /* Awaiting collection by parent. */ >> #define SWAIT 6 /* Waiting for interrupt. */ >> #define SLOCK 7 /* Blocked on a lock. */ >> [...] >> There is also the issue that: >> >> SIDL is misidentified as: sleep >> SRUN is misidentified as: ABANDONED (WAIT) >> SSLEEP is misidentified as: run >> SZOMB is misidentified as: start >> SWAIT is misidentified as: zombie >> SLOCK is out of bounds (as indicated above). >> >> So the sort order in top is not as documented. >> >> >> For reference, from sys/kern/kern_proc.c : >> >> if (p->p_state == PRS_NORMAL) { /* approximate. */ >> if (TD_ON_RUNQ(td) || >> TD_CAN_RUN(td) || >> TD_IS_RUNNING(td)) { >> kp->ki_stat = SRUN; >> } else if (P_SHOULDSTOP(p)) { >> kp->ki_stat = SSTOP; >> } else if (TD_IS_SLEEPING(td)) { >> kp->ki_stat = SSLEEP; >> } else if (TD_ON_LOCK(td)) { >> kp->ki_stat = SLOCK; >> } else { >> kp->ki_stat = SWAIT; >> } >> } else if (p->p_state == PRS_ZOMBIE) { >> kp->ki_stat = SZOMB; >> } else { >> kp->ki_stat = SIDL; >> } > https://reviews.freebsd.org/D40607   I rarely comment here and hesitate to now, but out of curiosity I looked at the original report and at the chain of commits.   It appears to me that in making the code more readable the final result may have accidentally inverted the sort order from the intended values (mostly.)  A casual test wouldn't show this, as unless the sort order in top were changed, normally it would only come into play for two processes with equal CPU % and ticks which seems unlikely.   Perhaps I'm simply misreading the original which was itself confused on the index values, but it appears from the original *comments*, as opposed to the effects, that higher values are intended to sort as higher list priority. For example, run was originally supposed to be mapped to 6 (largest value), start to 5, and so on down to zombie mapped to 2 and WAIT to 1.   The rewrite with designated initializers in  rGbc2ac2585aa8 now maps - in descending value order -  [SSLEEP] = 6, [SSTOP] = 5, [SWAIT] = 4 ... [SRUN] = 1.   As long as this is being fixed, shouldn't it read more like this: /*  *  proc_compare - comparison function for "qsort"  *    Compares the resource consumption of two processes using five  *    distinct keys.  The keys (in descending order of importance) are:  *    percent cpu, cpu ticks, state, resident set size, total virtual  *    memory usage.  The process states are ordered as follows (from most  *    to least important):  run, zombie, idle, interrupt wait, stop, sleep.  *    The array declaration below maps a process state index into a  *    number that reflects this ordering.  */ static const int sorted_state[] = {     [SRUN] =    6,    /* running/runnable    */     [SZOMB] =    5,    /* zombie        */     [SIDL] =    4,    /* being created    */     [SWAIT] =    3,    /* intr            */     [SSTOP] =    2,    /* stopped/suspended    */     [SSLEEP] =    1,    /* sleeping        */   I haven't read the entire context of how the values get used in the comparison, so it's possible I have this wrong.   I'm assuming where SIDL and SWAIT should fall based on the revised comments in rGd636fc5bd1e2 and the assumption that the original comment "(from least to most important)" was misread as "(from most to least important)" as I think one would normally want to see the "run" processes ahead of the "sleep" processes etc.   Best regards,   -- Clifton (My apologies if this comes out garbled, as seemingly I have to fight Thunderbird to edit for plain text output.) -- Clifton Royston