From owner-svn-src-head@freebsd.org Fri Nov 24 10:57:31 2017 Return-Path: Delivered-To: svn-src-head@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 4FFFADDF99F; Fri, 24 Nov 2017 10:57:31 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id DD80E6F498; Fri, 24 Nov 2017 10:57:30 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id vAOAvKDX004525 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 24 Nov 2017 12:57:20 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua vAOAvKDX004525 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id vAOAvKAB004524; Fri, 24 Nov 2017 12:57:20 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 24 Nov 2017 12:57:20 +0200 From: Konstantin Belousov To: Bruce Evans Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326073 - head/usr.bin/systat Message-ID: <20171124105720.GW2272@kib.kiev.ua> References: <201711211955.vALJtWhg047906@repo.freebsd.org> <20171122071838.R1172@besplex.bde.org> <20171122103917.GS2272@kib.kiev.ua> <20171123021646.M1933@besplex.bde.org> <20171122220538.GT2272@kib.kiev.ua> <20171123224032.A992@besplex.bde.org> <20171123151849.GU2272@kib.kiev.ua> <20171124184535.E980@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171124184535.E980@besplex.bde.org> User-Agent: Mutt/1.9.1 (2017-09-22) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Nov 2017 10:57:31 -0000 On Fri, Nov 24, 2017 at 08:15:06PM +1100, Bruce Evans wrote: > On Thu, 23 Nov 2017, Konstantin Belousov wrote: > > > On Fri, Nov 24, 2017 at 12:10:09AM +1100, Bruce Evans wrote: > >> On Thu, 23 Nov 2017, Konstantin Belousov wrote: > >* ... > >>> Below is the cast to uintmax_t and unsigned format for sysctl(8). > >> > >> This adds style bugs by expanding lines from length 79 to 81. > >> > >>> diff --git a/sbin/sysctl/sysctl.c b/sbin/sysctl/sysctl.c > >>> index e1bf4e31914..92685a8171b 100644 > >>> --- a/sbin/sysctl/sysctl.c > >>> +++ b/sbin/sysctl/sysctl.c > >* ... > >> All of the casts to uintmax_t can be avoided be avoided by changing the type > >> of pageKilo from int to uintmax_t. > >> > >> Better, do the conversion in a function-like macro pgtokb() as is done in > >> all (?) other utilities, but without so many overflow bugs as in most other > >> utiities: > >> > >> #define pgtok(p) ((uintmax_t)(p) * pageKilo) > > Amusingly there is already MD macro in machine/param.h with the same name > > and same intent, but as you formulate it, sloppy implementation. It uses > > unsigned long cast on almost all 64bit arches, except powerpc. For 32bit > > arches, the cast is not done, unfortunately. > > I already pointed out the system pgtok(). > > It was almost correct to cast to u_int on all arches because the > implementation can know the size of all its page counters. That was int > or u_int, and the expansion factor is small. In practice, 32-bit systems > never have enough memory to overflow in K (that happens at 4TB), and > 64-bit systems that overflow in K are close to overflowing the page > counters (that happens at 16TB with 4K-pages). > > The pgtok() is just unusable because the page size can vary. No, it is unusable only due to the implementation not ensuring the consistent output type. > > Perhaps it is a design error to allow the page size to vary or be > anything except 512 in APIs, just like for disk sector sizes. Most > disk APIs uses units of bytes or 512-blocks. The physical size may > be different. Applications need to know the physical memory page size > even less than they need to know the physical sector size. Not everybody share the warm memories about VAX. I think there were no single significant architecture with hardware support for virtual memory, after the VAX, which used less than 4K sized pages. > > Here are my old fixes for this function (to clean it up before removing it): I picked some of this, mainly I do not want to change the output format. I am sure that there are scripts around which parse it. diff --git a/sbin/sysctl/sysctl.c b/sbin/sysctl/sysctl.c index e1bf4e31914..de7fd67f258 100644 --- a/sbin/sysctl/sysctl.c +++ b/sbin/sysctl/sysctl.c @@ -608,33 +608,33 @@ S_timeval(size_t l2, void *p) static int S_vmtotal(size_t l2, void *p) { - struct vmtotal *v = (struct vmtotal *)p; - int pageKilo = getpagesize() / 1024; + struct vmtotal *v; + int pageKilo; if (l2 != sizeof(*v)) { warnx("S_vmtotal %zu != %zu", l2, sizeof(*v)); return (1); } - printf( - "\nSystem wide totals computed every five seconds:" + v = p; + pageKilo = getpagesize() / 1024; + +#define pg2k(a) ((uintmax_t)(a) * pageKilo) + printf("\nSystem wide totals computed every five seconds:" " (values in kilobytes)\n"); printf("===============================================\n"); - printf( - "Processes:\t\t(RUNQ: %hd Disk Wait: %hd Page Wait: " - "%hd Sleep: %hd)\n", + printf("Processes:\t\t(RUNQ: %d Disk Wait: %d Page Wait: " + "%d Sleep: %d)\n", v->t_rq, v->t_dw, v->t_pw, v->t_sl); - printf( - "Virtual Memory:\t\t(Total: %jdK Active: %jdK)\n", - (intmax_t)v->t_vm * pageKilo, (intmax_t)v->t_avm * pageKilo); - printf("Real Memory:\t\t(Total: %jdK Active: %jdK)\n", - (intmax_t)v->t_rm * pageKilo, (intmax_t)v->t_arm * pageKilo); - printf("Shared Virtual Memory:\t(Total: %jdK Active: %jdK)\n", - (intmax_t)v->t_vmshr * pageKilo, (intmax_t)v->t_avmshr * pageKilo); - printf("Shared Real Memory:\t(Total: %jdK Active: %jdK)\n", - (intmax_t)v->t_rmshr * pageKilo, (intmax_t)v->t_armshr * pageKilo); - printf("Free Memory:\t%jdK", (intmax_t)v->t_free * pageKilo); - + printf("Virtual Memory:\t\t(Total: %juK Active: %juK)\n", + pg2k(v->t_vm), pg2k(v->t_avm)); + printf("Real Memory:\t\t(Total: %juK Active: %juK)\n", + pg2k(v->t_rm), pg2k(v->t_arm)); + printf("Shared Virtual Memory:\t(Total: %juK Active: %juK)\n", + pg2k(v->t_vmshr), pg2k(v->t_avmshr)); + printf("Shared Real Memory:\t(Total: %juK Active: %juK)\n", + pg2k(v->t_rmshr), pg2k(v->t_armshr)); + printf("Free Memory:\t%juK", pg2k(v->t_free)); return (0); } @@ -695,8 +695,9 @@ S_efi_map(size_t l2, void *p) type = types[map->md_type]; else type = ""; - printf("\n%23s %012lx %12p %08lx ", type, map->md_phys, - map->md_virt, map->md_pages); + printf("\n%23s %012jx %12p %08jx ", type, + (uintmax_t)map->md_phys, map->md_virt, + (uintmax_t)map->md_pages); if (map->md_attr & EFI_MD_ATTR_UC) printf("UC "); if (map->md_attr & EFI_MD_ATTR_WC)