Date: Fri, 24 Nov 2017 12:57:20 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> 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> In-Reply-To: <20171124184535.E980@besplex.bde.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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 = "<INVALID>";
- 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)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171124105720.GW2272>