devel/git: cpu and memory tracking, LIB_DEPENDS

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

devel/git: cpu and memory tracking, LIB_DEPENDS

Jeremie Courreges-Anglas-2


I'm sitting on a few changes in devel/git since some time already.

- nghttp2 should not be listed here IMO.  It really is a dep of libcurl,
  git itself doesn't use directly the nghttp API.

- sysctl(HW_PHYSMEM) wants an int.  That doesn't work well if you have
  a decent amount or ram.  What doesn't work well either is storing that
  int in the first bytes of an uninitialized int64_t stack variable.
  Let's properly use HW_PHYSMEM64 instead.  I have left the HW_PHYSMEM
  code path so that the diff can be pushed upstream as-is without too
  many questions, but I'm not sure it makes much sense.

- with hw.smt=0, git may use more threads than the number of available
  cpu cores.  I initally noticed this when testing the hw.smt diffs on
  my x230 a few releases ago.  The simple solution is to reorder the
  code to avoid using HW_NCPU.

ok?


Index: Makefile
===================================================================
RCS file: /cvs/ports/devel/git/Makefile,v
retrieving revision 1.204
diff -u -p -r1.204 Makefile
--- Makefile 25 Oct 2019 08:03:51 -0000 1.204
+++ Makefile 3 Nov 2019 10:23:16 -0000
@@ -5,7 +5,7 @@ COMMENT-svn = subversion interoperabilit
 COMMENT-x11 = graphical tools
 
 V = 2.23.0
-REVISION = 0
+REVISION = 1
 DISTNAME = git-${V}
 PKGNAME-main = ${DISTNAME}
 PKGNAME-svn = git-svn-${V}
@@ -49,8 +49,7 @@ WANTLIB-main = c charset crypto curl ex
 RUN_DEPENDS-main = devel/cvsps \
  devel/p5-Error
 LIB_DEPENDS-main = devel/gettext,-runtime \
- net/curl \
- www/nghttp2
+ net/curl
 
 RUN_DEPENDS-svn = ${BASE_PKGPATH} \
  devel/subversion,-perl \
Index: patches/patch-builtin_gc_c
===================================================================
RCS file: patches/patch-builtin_gc_c
diff -N patches/patch-builtin_gc_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-builtin_gc_c 3 Nov 2019 10:23:16 -0000
@@ -0,0 +1,38 @@
+$OpenBSD$
+
+HW_PHYSMEM asks for an int, not an int64_t.
+Use HW_PHYSMEM64 to handle ram size > 2GB.
+
+Index: builtin/gc.c
+--- builtin/gc.c.orig
++++ builtin/gc.c
+@@ -244,7 +244,7 @@ static uint64_t total_ram(void)
+
+ if (!sysinfo(&si))
+ return si.totalram;
+-#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM))
++#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
+ int64_t physical_memory;
+ int mib[2];
+ size_t length;
+@@ -253,9 +253,19 @@ static uint64_t total_ram(void)
+ # if defined(HW_MEMSIZE)
+ mib[1] = HW_MEMSIZE;
+ # else
+- mib[1] = HW_PHYSMEM;
++ mib[1] = HW_PHYSMEM64;
+ # endif
+ length = sizeof(int64_t);
++ if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
++ return physical_memory;
++#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
++ int physical_memory;
++ int mib[2];
++ size_t length;
++
++ mib[0] = CTL_HW;
++ mib[1] = HW_PHYSMEM;
++ length = sizeof(int);
+ if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
+ return physical_memory;
+ #elif defined(GIT_WINDOWS_NATIVE)
Index: patches/patch-thread-utils_c
===================================================================
RCS file: patches/patch-thread-utils_c
diff -N patches/patch-thread-utils_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-thread-utils_c 3 Nov 2019 10:23:16 -0000
@@ -0,0 +1,32 @@
+$OpenBSD$
+
+Use sysconf(_SC_NPROCESSORS_ONLN) to properly omit disabled smt cores.
+
+Index: thread-utils.c
+--- thread-utils.c.orig
++++ thread-utils.c
+@@ -25,9 +25,10 @@ int online_cpus(void)
+ #else
+ #ifdef _SC_NPROCESSORS_ONLN
+ long ncpus;
+-#endif
+
+-#ifdef GIT_WINDOWS_NATIVE
++ if ((ncpus = (long)sysconf(_SC_NPROCESSORS_ONLN)) > 0)
++ return (int)ncpus;
++#elif defined(GIT_WINDOWS_NATIVE)
+ SYSTEM_INFO info;
+ GetSystemInfo(&info);
+
+@@ -55,11 +56,6 @@ int online_cpus(void)
+ if (!sysctl(mib, 2, &cpucount, &len, NULL, 0))
+ return cpucount;
+ #endif /* defined(HAVE_BSD_SYSCTL) && defined(HW_NCPU) */
+-
+-#ifdef _SC_NPROCESSORS_ONLN
+- if ((ncpus = (long)sysconf(_SC_NPROCESSORS_ONLN)) > 0)
+- return (int)ncpus;
+-#endif
+
+ return 1;
+ #endif


--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Klemens Nanni-2
On Sun, Nov 03, 2019 at 12:17:13PM +0100, Jeremie Courreges-Anglas wrote:
> I'm sitting on a few changes in devel/git since some time already.
Thanks, I never noticed these until now.

> - nghttp2 should not be listed here IMO.  It really is a dep of libcurl,
>   git itself doesn't use directly the nghttp API.
This might just be another remnant just like the recent removal of rsync
as RDEP.

> - sysctl(HW_PHYSMEM) wants an int.  That doesn't work well if you have
>   a decent amount or ram.  What doesn't work well either is storing that
>   int in the first bytes of an uninitialized int64_t stack variable.
>   Let's properly use HW_PHYSMEM64 instead.  I have left the HW_PHYSMEM
>   code path so that the diff can be pushed upstream as-is without too
>   many questions, but I'm not sure it makes much sense.
HW_PHYSMEM64 should really be used unconditionally, no?  Can we actually
reach the case where HAVE_BSD_SYSCTL and HW_PHYSMEM are defined but
HW_PHYSMEM64 is not?  What about other BSDs?

> - with hw.smt=0, git may use more threads than the number of available
>   cpu cores.  I initally noticed this when testing the hw.smt diffs on
>   my x230 a few releases ago.  The simple solution is to reorder the
>   code to avoid using HW_NCPU.
This can be easily seen with `top -H -C -g git' and `git gc --aggressive'
on any repository;  without the patch it always spawns four packer
threads regardless of `hw.smt'.  With your diff and SMT turned off on my
X230 git only starts two not four additional threads.

OK kn but as per above, I'm not sure about the memory bits.

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Stuart Henderson
On 2019/11/03 14:57, Klemens Nanni wrote:
> On Sun, Nov 03, 2019 at 12:17:13PM +0100, Jeremie Courreges-Anglas wrote:
> > I'm sitting on a few changes in devel/git since some time already.
> Thanks, I never noticed these until now.
>
> > - nghttp2 should not be listed here IMO.  It really is a dep of libcurl,
> >   git itself doesn't use directly the nghttp API.
> This might just be another remnant just like the recent removal of rsync
> as RDEP.

This happens a few times, people add nghttp2 as LIB_DEPENDS when it's
often not used by the port itself, only by libcurl. (Some programs do use
nghttp2 directly, including snort and wireshark, but this is not so common).

> > - sysctl(HW_PHYSMEM) wants an int.  That doesn't work well if you have
> >   a decent amount or ram.  What doesn't work well either is storing that
> >   int in the first bytes of an uninitialized int64_t stack variable.
> >   Let's properly use HW_PHYSMEM64 instead.  I have left the HW_PHYSMEM
> >   code path so that the diff can be pushed upstream as-is without too
> >   many questions, but I'm not sure it makes much sense.
> HW_PHYSMEM64 should really be used unconditionally, no?  Can we actually
> reach the case where HAVE_BSD_SYSCTL and HW_PHYSMEM are defined but
> HW_PHYSMEM64 is not?  What about other BSDs?

I think both should be left, FreeBSD doesn't have HW_PHYSMEM64.

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Jeremie Courreges-Anglas-2
On Sun, Nov 03 2019, Stuart Henderson <[hidden email]> wrote:

> On 2019/11/03 14:57, Klemens Nanni wrote:
>> On Sun, Nov 03, 2019 at 12:17:13PM +0100, Jeremie Courreges-Anglas wrote:
>> > I'm sitting on a few changes in devel/git since some time already.
>> Thanks, I never noticed these until now.
>>
>> > - nghttp2 should not be listed here IMO.  It really is a dep of libcurl,
>> >   git itself doesn't use directly the nghttp API.
>> This might just be another remnant just like the recent removal of rsync
>> as RDEP.
>
> This happens a few times, people add nghttp2 as LIB_DEPENDS when it's
> often not used by the port itself, only by libcurl. (Some programs do use
> nghttp2 directly, including snort and wireshark, but this is not so common).

And I think that's what happened in devel/git.

>> > - sysctl(HW_PHYSMEM) wants an int.  That doesn't work well if you have
>> >   a decent amount or ram.  What doesn't work well either is storing that
>> >   int in the first bytes of an uninitialized int64_t stack variable.
>> >   Let's properly use HW_PHYSMEM64 instead.  I have left the HW_PHYSMEM
>> >   code path so that the diff can be pushed upstream as-is without too
>> >   many questions, but I'm not sure it makes much sense.
>> HW_PHYSMEM64 should really be used unconditionally, no?  Can we actually
>> reach the case where HAVE_BSD_SYSCTL and HW_PHYSMEM are defined but
>> HW_PHYSMEM64 is not?  What about other BSDs?
>
> I think both should be left, FreeBSD doesn't have HW_PHYSMEM64.

Yeah that's the reason why I left that code path alone.  I hate it when
other *BSD systems push "portability" diffs upstream which negatively
affect us.

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Klemens Nanni-2
In reply to this post by Stuart Henderson
On Sun, Nov 03, 2019 at 02:22:19PM +0000, Stuart Henderson wrote:
> I think both should be left, FreeBSD doesn't have HW_PHYSMEM64.
I see, thanks.  Just looked and as presumed NetBSD has PHYSMEM64 while
Dragonfly does not just like FreeBSD, so the seems fine as is.

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Jan Beich
In reply to this post by Jeremie Courreges-Anglas-2
Jeremie Courreges-Anglas <[hidden email]> writes:

> ++#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
> + int64_t physical_memory;

HW_MEMSIZE and HW_PHYSMEM64 return uint64_t, not int64_t.

> ++#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
> ++ int physical_memory;

HW_PHYSMEM returns u_long (unsigned long) on DragonFly and FreeBSD.
int or signed long may upset -fsanitize=integer on 32-bit archs.

Note, the code can be simplified via sysconf(3).

--- builtin/gc.c 2019-11-04 05:07:07 UTC
+++ builtin/gc.c
@@ -243,20 +243,27 @@ static uint64_t total_ram(void)
 
  if (!sysinfo(&si))
  return si.totalram;
-#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM))
- int64_t physical_memory;
- int mib[2];
- size_t length;
-
- mib[0] = CTL_HW;
+#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64) || defined(HW_PHYSMEM))
+# if defined(HW_MEMSIZE) || defined(HW_PHYSMEM64)
+ uint64_t physical_memory;
+# else
+ u_long physical_memory;
+# endif
+ int mib[2] = {
+ CTL_HW,
 # if defined(HW_MEMSIZE)
- mib[1] = HW_MEMSIZE;
+ HW_MEMSIZE,
+# elif defined(HW_PHYSMEM64)
+ HW_PHYSMEM64,
 # else
- mib[1] = HW_PHYSMEM;
+ HW_PHYSMEM,
 # endif
- length = sizeof(int64_t);
+ };
+ size_t length = sizeof(mib);
  if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
  return physical_memory;
+#elif defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
+ return (uint64_t)sysconf(_SC_PHYS_PAGES) * sysconf(_SC_PAGESIZE);
 #elif defined(GIT_WINDOWS_NATIVE)
  MEMORYSTATUSEX memInfo;
 

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Kurt Miller-4
On Tue, 2019-11-05 at 09:17 +0100, Jan Beich wrote:

> Jeremie Courreges-Anglas <[hidden email]> writes:
>
> >
> > ++#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
> > +  int64_t physical_memory;
> HW_MEMSIZE and HW_PHYSMEM64 return uint64_t, not int64_t.
>
> >
> > ++#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
> > ++ int physical_memory;
> HW_PHYSMEM returns u_long (unsigned long) on DragonFly and FreeBSD.
> int or signed long may upset -fsanitize=integer on 32-bit archs.
>
> Note, the code can be simplified via sysconf(3).
>
> --- builtin/gc.c 2019-11-04 05:07:07 UTC
> +++ builtin/gc.c
> @@ -243,20 +243,27 @@ static uint64_t total_ram(void)
>  
>   if (!sysinfo(&si))
>   return si.totalram;
> -#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM))
> - int64_t physical_memory;
> - int mib[2];
> - size_t length;
> -
> - mib[0] = CTL_HW;
> +#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64) || defined(HW_PHYSMEM))
> +# if defined(HW_MEMSIZE) || defined(HW_PHYSMEM64)
> + uint64_t physical_memory;
> +# else
> + u_long physical_memory;
> +# endif
> + int mib[2] = {
> + CTL_HW,
>  # if defined(HW_MEMSIZE)
> - mib[1] = HW_MEMSIZE;
> + HW_MEMSIZE,
> +# elif defined(HW_PHYSMEM64)
> + HW_PHYSMEM64,
>  # else
> - mib[1] = HW_PHYSMEM;
> + HW_PHYSMEM,
>  # endif
> - length = sizeof(int64_t);
> + };
> + size_t length = sizeof(mib);

size_t length = sizeof(physical_memory);

>   if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
>   return physical_memory;
> +#elif defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
> + return (uint64_t)sysconf(_SC_PHYS_PAGES) * sysconf(_SC_PAGESIZE);
>  #elif defined(GIT_WINDOWS_NATIVE)
>   MEMORYSTATUSEX memInfo;
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Jan Beich
Kurt Miller <[hidden email]> writes:

> On Tue, 2019-11-05 at 09:17 +0100, Jan Beich wrote:
>
>> Jeremie Courreges-Anglas <[hidden email]> writes:
>>
>> >
>> > ++#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
>> > +  int64_t physical_memory;
>> HW_MEMSIZE and HW_PHYSMEM64 return uint64_t, not int64_t.
>>
>> >
>> > ++#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
>> > ++ int physical_memory;
>> HW_PHYSMEM returns u_long (unsigned long) on DragonFly and FreeBSD.
>> int or signed long may upset -fsanitize=integer on 32-bit archs.
>>
>> Note, the code can be simplified via sysconf(3).
>>
>> --- builtin/gc.c 2019-11-04 05:07:07 UTC
>> +++ builtin/gc.c
>> @@ -243,20 +243,27 @@ static uint64_t total_ram(void)
>>  
>>   if (!sysinfo(&si))
>>   return si.totalram;
>> -#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM))
>> - int64_t physical_memory;
>> - int mib[2];
>> - size_t length;
>> -
>> - mib[0] = CTL_HW;
>> +#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64) || defined(HW_PHYSMEM))
>> +# if defined(HW_MEMSIZE) || defined(HW_PHYSMEM64)
>> + uint64_t physical_memory;
>> +# else
>> + u_long physical_memory;
>> +# endif
>> + int mib[2] = {
>> + CTL_HW,
>>  # if defined(HW_MEMSIZE)
>> - mib[1] = HW_MEMSIZE;
>> + HW_MEMSIZE,
>> +# elif defined(HW_PHYSMEM64)
>> + HW_PHYSMEM64,
>>  # else
>> - mib[1] = HW_PHYSMEM;
>> + HW_PHYSMEM,
>>  # endif
>> - length = sizeof(int64_t);
>> + };
>> + size_t length = sizeof(mib);
>
> size_t length = sizeof(physical_memory);

Sorry. sizeof(int[2]) > sizeof(unsigned long) on i386, so sysctl(3)
could overflow &physical_memory iff FreeBSD kernel tried to return
larger value or padded it with junk/zeros.

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Kurt Miller-4
On Tue, 2019-11-05 at 14:44 +0100, Jan Beich wrote:

> Kurt Miller <[hidden email]> writes:
>
> >
> > On Tue, 2019-11-05 at 09:17 +0100, Jan Beich wrote:
> >
> > >
> > > Jeremie Courreges-Anglas <[hidden email]> writes:
> > >
> > > >
> > > >
> > > > ++#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
> > > > +  int64_t physical_memory;
> > > HW_MEMSIZE and HW_PHYSMEM64 return uint64_t, not int64_t.
> > >
> > > >
> > > >
> > > > ++#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
> > > > ++ int physical_memory;
> > > HW_PHYSMEM returns u_long (unsigned long) on DragonFly and FreeBSD.
> > > int or signed long may upset -fsanitize=integer on 32-bit archs.
> > >
> > > Note, the code can be simplified via sysconf(3).
> > >
> > > --- builtin/gc.c 2019-11-04 05:07:07 UTC
> > > +++ builtin/gc.c
> > > @@ -243,20 +243,27 @@ static uint64_t total_ram(void)
> > >  
> > >   if (!sysinfo(&si))
> > >   return si.totalram;
> > > -#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM))
> > > - int64_t physical_memory;
> > > - int mib[2];
> > > - size_t length;
> > > -
> > > - mib[0] = CTL_HW;
> > > +#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64) || defined(HW_PHYSMEM))
> > > +# if defined(HW_MEMSIZE) || defined(HW_PHYSMEM64)
> > > + uint64_t physical_memory;
> > > +# else
> > > + u_long physical_memory;
> > > +# endif
> > > + int mib[2] = {
> > > + CTL_HW,
> > >  # if defined(HW_MEMSIZE)
> > > - mib[1] = HW_MEMSIZE;
> > > + HW_MEMSIZE,
> > > +# elif defined(HW_PHYSMEM64)
> > > + HW_PHYSMEM64,
> > >  # else
> > > - mib[1] = HW_PHYSMEM;
> > > + HW_PHYSMEM,
> > >  # endif
> > > - length = sizeof(int64_t);
> > > + };
> > > + size_t length = sizeof(mib);
> > size_t length = sizeof(physical_memory);
> Sorry. sizeof(int[2]) > sizeof(unsigned long) on i386, so sysctl(3)
> could overflow &physical_memory iff FreeBSD kernel tried to return
> larger value or padded it with junk/zeros.

I think you are confused. The fourth argument to sysctl(2) is
the address of a size_t that contains sizeof the third argument
before the call. Also HW_PHYSMEM64 is int64_t on OpenBSD.

-Kurt


Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Jan Beich
Kurt Miller <[hidden email]> writes:

> On Tue, 2019-11-05 at 14:44 +0100, Jan Beich wrote:
>
>> Kurt Miller <[hidden email]> writes:
>>
>> >
>> > On Tue, 2019-11-05 at 09:17 +0100, Jan Beich wrote:
>> >
>> > >
>> > > Jeremie Courreges-Anglas <[hidden email]> writes:
>> > >
>> > > >
>> > > >
>> > > > ++#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
>> > > > + int64_t physical_memory;
>> > > HW_MEMSIZE and HW_PHYSMEM64 return uint64_t, not int64_t.
>> > >
>> > > >
>> > > >
>> > > > ++#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
>> > > > ++ int physical_memory;
>> > > HW_PHYSMEM returns u_long (unsigned long) on DragonFly and FreeBSD.
>> > > int or signed long may upset -fsanitize=integer on 32-bit archs.
>> > >
>> > > Note, the code can be simplified via sysconf(3).
>> > >
>> > > --- builtin/gc.c 2019-11-04 05:07:07 UTC
>> > > +++ builtin/gc.c
>> > > @@ -243,20 +243,27 @@ static uint64_t total_ram(void)
>> > >  
>> > >   if (!sysinfo(&si))
>> > >   return si.totalram;
>> > > -#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM))
>> > > - int64_t physical_memory;
>> > > - int mib[2];
>> > > - size_t length;
>> > > -
>> > > - mib[0] = CTL_HW;
>> > > +#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64) || defined(HW_PHYSMEM))
>> > > +# if defined(HW_MEMSIZE) || defined(HW_PHYSMEM64)
>> > > + uint64_t physical_memory;
>> > > +# else
>> > > + u_long physical_memory;
>> > > +# endif
>> > > + int mib[2] = {
>> > > + CTL_HW,
>> > >  # if defined(HW_MEMSIZE)
>> > > - mib[1] = HW_MEMSIZE;
>> > > + HW_MEMSIZE,
>> > > +# elif defined(HW_PHYSMEM64)
>> > > + HW_PHYSMEM64,
>> > >  # else
>> > > - mib[1] = HW_PHYSMEM;
>> > > + HW_PHYSMEM,
>> > >  # endif
>> > > - length = sizeof(int64_t);
>> > > + };
>> > > + size_t length = sizeof(mib);
>> > size_t length = sizeof(physical_memory);
>> Sorry. sizeof(int[2]) > sizeof(unsigned long) on i386, so sysctl(3)
>> could overflow &physical_memory iff FreeBSD kernel tried to return
>> larger value or padded it with junk/zeros.
>
> I think you are confused. The fourth argument to sysctl(2) is
> the address of a size_t that contains sizeof the third argument
> before the call. Also HW_PHYSMEM64 is int64_t on OpenBSD.

I didn't disagree. My reply was an attempt to understand what may go
wrong at runtime as the typo didn't trigger -fsanitize=address.

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Kurt Miller-3
On Tue, 2019-11-05 at 15:48 +0100, Jan Beich wrote:

> Kurt Miller <[hidden email]> writes:
>
> >
> > On Tue, 2019-11-05 at 14:44 +0100, Jan Beich wrote:
> >
> > >
> > > Kurt Miller <[hidden email]> writes:
> > >
> > > >
> > > >
> > > > On Tue, 2019-11-05 at 09:17 +0100, Jan Beich wrote:
> > > >
> > > > >
> > > > >
> > > > > Jeremie Courreges-Anglas <[hidden email]> writes:
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > ++#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
> > > > > > +  int64_t physical_memory;
> > > > > HW_MEMSIZE and HW_PHYSMEM64 return uint64_t, not int64_t.
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > ++#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
> > > > > > ++ int physical_memory;
> > > > > HW_PHYSMEM returns u_long (unsigned long) on DragonFly and FreeBSD.
> > > > > int or signed long may upset -fsanitize=integer on 32-bit archs.
> > > > >
> > > > > Note, the code can be simplified via sysconf(3).
> > > > >
> > > > > --- builtin/gc.c 2019-11-04 05:07:07 UTC
> > > > > +++ builtin/gc.c
> > > > > @@ -243,20 +243,27 @@ static uint64_t total_ram(void)
> > > > >  
> > > > >   if (!sysinfo(&si))
> > > > >   return si.totalram;
> > > > > -#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM))
> > > > > - int64_t physical_memory;
> > > > > - int mib[2];
> > > > > - size_t length;
> > > > > -
> > > > > - mib[0] = CTL_HW;
> > > > > +#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64) || defined(HW_PHYSMEM))
> > > > > +# if defined(HW_MEMSIZE) || defined(HW_PHYSMEM64)
> > > > > + uint64_t physical_memory;
> > > > > +# else
> > > > > + u_long physical_memory;
> > > > > +# endif
> > > > > + int mib[2] = {
> > > > > + CTL_HW,
> > > > >  # if defined(HW_MEMSIZE)
> > > > > - mib[1] = HW_MEMSIZE;
> > > > > + HW_MEMSIZE,
> > > > > +# elif defined(HW_PHYSMEM64)
> > > > > + HW_PHYSMEM64,
> > > > >  # else
> > > > > - mib[1] = HW_PHYSMEM;
> > > > > + HW_PHYSMEM,
> > > > >  # endif
> > > > > - length = sizeof(int64_t);
> > > > > + };
> > > > > + size_t length = sizeof(mib);
> > > > size_t length = sizeof(physical_memory);
> > > Sorry. sizeof(int[2]) > sizeof(unsigned long) on i386, so sysctl(3)
> > > could overflow &physical_memory iff FreeBSD kernel tried to return
> > > larger value or padded it with junk/zeros.
> > I think you are confused. The fourth argument to sysctl(2) is
> > the address of a size_t that contains sizeof the third argument
> > before the call. Also HW_PHYSMEM64 is int64_t on OpenBSD.
> I didn't disagree. My reply was an attempt to understand what may go
> wrong at runtime as the typo didn't trigger -fsanitize=address.

Oh, sorry. I misunderstood your reply and thought it was
trying to explain why sizeof(mib) was correct.

-Kurt

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Jeremie Courreges-Anglas-2
In reply to this post by Kurt Miller-4

Back to this,

On Tue, Nov 05 2019, Kurt Miller <[hidden email]> wrote:
> On Tue, 2019-11-05 at 09:17 +0100, Jan Beich wrote:
>> Jeremie Courreges-Anglas <[hidden email]> writes:
>>
>> >
>> > ++#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
>> > +  int64_t physical_memory;
>> HW_MEMSIZE and HW_PHYSMEM64 return uint64_t, not int64_t.

Hrm, we document this as int64_t, even though the value is obviously not
negative (that's the type used by sysctl_rdquad).

NetBSD documents HW_PHYSMEM64 as "quad" without mentioning signedness,
the underlying code in the kernel uses u_quad_t.

Apple seems to document HW_MEMSIZE as an "integer" in its manpage, even
though the kernel variable is an uint64_t.

>> > ++#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
>> > ++ int physical_memory;
>> HW_PHYSMEM returns u_long (unsigned long) on DragonFly and FreeBSD.

I'll trust you on this, sadly... sysctl.3 says only "integer", and
sys/sysctl.h says "int", which doesn't help much.

  https://svnweb.freebsd.org/base/head/sys/sys/sysctl.h
  https://www.freebsd.org/cgi/man.cgi?query=sysctl&apropos=0&sektion=3

The whole situation is a mess.  I guess the most reasonable approach
using sysctl is what Jan proposed, use uint64_t for HW_PHYSMEM64 and
HW_MEMSIZE, and fall back on HW_PHYSMEM + u_long on FreeBSD.  I don't
think signedness would cause any issue.

>> int or signed long may upset -fsanitize=integer on 32-bit archs.
>>
>> Note, the code can be simplified via sysconf(3).

Indeed.  Initially I thought the sysctl hw.* code could be fixed; but now
that I've rolled into this mud I think sysconf should be promoted as
a simpler and more portable way to handle this, even with _SC_PHYS_PAGES
not being defined by POSIX.  The sysinfo code path used on Linux could
also be deleted this way.

This should be discussed with upstream, but seeing adoption on our side
could help convince them.  Jan, would FreeBSD be happy with this
approach?  Benoit, Klemens, Kurt: thoughts/oks?

If people like this approach I can try to push it upstream, along with
the ncpu diff.


Index: Makefile
===================================================================
RCS file: /cvs/ports/devel/git/Makefile,v
retrieving revision 1.206
diff -u -p -r1.206 Makefile
--- Makefile 3 Nov 2019 15:27:44 -0000 1.206
+++ Makefile 7 Nov 2019 14:47:18 -0000
@@ -5,7 +5,7 @@ COMMENT-svn = subversion interoperabilit
 COMMENT-x11 = graphical tools
 
 V = 2.23.0
-REVISION = 2
+REVISION = 3
 DISTNAME = git-${V}
 PKGNAME-main = ${DISTNAME}
 PKGNAME-svn = git-svn-${V}
Index: patches/patch-builtin_gc_c
===================================================================
RCS file: /cvs/ports/devel/git/patches/patch-builtin_gc_c,v
retrieving revision 1.1
diff -u -p -r1.1 patch-builtin_gc_c
--- patches/patch-builtin_gc_c 3 Nov 2019 15:25:22 -0000 1.1
+++ patches/patch-builtin_gc_c 7 Nov 2019 14:47:18 -0000
@@ -1,38 +1,34 @@
 $OpenBSD: patch-builtin_gc_c,v 1.1 2019/11/03 15:25:22 jca Exp $
 
-HW_PHYSMEM asks for an int, not an int64_t.
-Use HW_PHYSMEM64 to handle ram size > 2GB.
+Replace sysctl HW_PHYSMEM/MEMSIZE/whatever madness with sysconf.
 
 Index: builtin/gc.c
 --- builtin/gc.c.orig
 +++ builtin/gc.c
-@@ -244,7 +244,7 @@ static uint64_t total_ram(void)
+@@ -244,20 +244,13 @@ static uint64_t total_ram(void)
 
  if (!sysinfo(&si))
  return si.totalram;
 -#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM))
-+#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
- int64_t physical_memory;
- int mib[2];
- size_t length;
-@@ -253,9 +253,19 @@ static uint64_t total_ram(void)
- # if defined(HW_MEMSIZE)
- mib[1] = HW_MEMSIZE;
- # else
+- int64_t physical_memory;
+- int mib[2];
+- size_t length;
++#elif defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
++ long phys_pages, pagesize;
+
+- mib[0] = CTL_HW;
+-# if defined(HW_MEMSIZE)
+- mib[1] = HW_MEMSIZE;
+-# else
 - mib[1] = HW_PHYSMEM;
-+ mib[1] = HW_PHYSMEM64;
- # endif
- length = sizeof(int64_t);
-+ if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
-+ return physical_memory;
-+#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
-+ int physical_memory;
-+ int mib[2];
-+ size_t length;
-+
-+ mib[0] = CTL_HW;
-+ mib[1] = HW_PHYSMEM;
-+ length = sizeof(int);
- if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
- return physical_memory;
+-# endif
+- length = sizeof(int64_t);
+- if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
+- return physical_memory;
++ phys_pages = sysconf(_SC_PHYS_PAGES);
++ pagesize = sysconf(_SC_PAGESIZE);
++ if (phys_pages != -1 && pagesize != -1)
++ return (uint64_t)phys_pages * (uint64_t)pagesize;
  #elif defined(GIT_WINDOWS_NATIVE)
+ MEMORYSTATUSEX memInfo;
+



--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Jeremie Courreges-Anglas-2
On Thu, Nov 07 2019, Jeremie Courreges-Anglas <[hidden email]> wrote:

> Back to this,
>
> On Tue, Nov 05 2019, Kurt Miller <[hidden email]> wrote:
>> On Tue, 2019-11-05 at 09:17 +0100, Jan Beich wrote:
>>> Jeremie Courreges-Anglas <[hidden email]> writes:
>>>
>>> >
>>> > ++#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
>>> > +  int64_t physical_memory;
>>> HW_MEMSIZE and HW_PHYSMEM64 return uint64_t, not int64_t.
>
> Hrm, we document this as int64_t, even though the value is obviously not
> negative (that's the type used by sysctl_rdquad).
>
> NetBSD documents HW_PHYSMEM64 as "quad" without mentioning signedness,
> the underlying code in the kernel uses u_quad_t.
>
> Apple seems to document HW_MEMSIZE as an "integer" in its manpage, even
> though the kernel variable is an uint64_t.
>
>>> > ++#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
>>> > ++ int physical_memory;
>>> HW_PHYSMEM returns u_long (unsigned long) on DragonFly and FreeBSD.
>
> I'll trust you on this, sadly... sysctl.3 says only "integer", and
> sys/sysctl.h says "int", which doesn't help much.
>
>   https://svnweb.freebsd.org/base/head/sys/sys/sysctl.h
>   https://www.freebsd.org/cgi/man.cgi?query=sysctl&apropos=0&sektion=3
>
> The whole situation is a mess.  I guess the most reasonable approach
> using sysctl is what Jan proposed, use uint64_t for HW_PHYSMEM64 and
> HW_MEMSIZE, and fall back on HW_PHYSMEM + u_long on FreeBSD.  I don't
> think signedness would cause any issue.
>
>>> int or signed long may upset -fsanitize=integer on 32-bit archs.
>>>
>>> Note, the code can be simplified via sysconf(3).
>
> Indeed.  Initially I thought the sysctl hw.* code could be fixed; but now
> that I've rolled into this mud I think sysconf should be promoted as
> a simpler and more portable way to handle this, even with _SC_PHYS_PAGES
> not being defined by POSIX.  The sysinfo code path used on Linux could
> also be deleted this way.
>
> This should be discussed with upstream, but seeing adoption on our side
> could help convince them.  Jan, would FreeBSD be happy with this
> approach?  Benoit, Klemens, Kurt: thoughts/oks?
>
> If people like this approach I can try to push it upstream, along with
> the ncpu diff.

Diff rebased on -current.


Index: Makefile
===================================================================
RCS file: /cvs/ports/devel/git/Makefile,v
retrieving revision 1.207
diff -u -p -r1.207 Makefile
--- Makefile 11 Nov 2019 10:12:32 -0000 1.207
+++ Makefile 11 Nov 2019 19:03:10 -0000
@@ -5,6 +5,7 @@ COMMENT-svn = subversion interoperabilit
 COMMENT-x11 = graphical tools
 
 V = 2.24.0
+REVISION = 0
 DISTNAME = git-${V}
 PKGNAME-main = ${DISTNAME}
 PKGNAME-svn = git-svn-${V}
Index: patches/patch-builtin_gc_c
===================================================================
RCS file: /cvs/ports/devel/git/patches/patch-builtin_gc_c,v
retrieving revision 1.2
diff -u -p -r1.2 patch-builtin_gc_c
--- patches/patch-builtin_gc_c 11 Nov 2019 10:12:33 -0000 1.2
+++ patches/patch-builtin_gc_c 11 Nov 2019 19:03:10 -0000
@@ -1,38 +1,34 @@
 $OpenBSD: patch-builtin_gc_c,v 1.2 2019/11/11 10:12:33 kn Exp $
 
-HW_PHYSMEM asks for an int, not an int64_t.
-Use HW_PHYSMEM64 to handle ram size > 2GB.
+Replace sysctl HW_PHYSMEM/MEMSIZE/whatever madness with sysconf.
 
 Index: builtin/gc.c
 --- builtin/gc.c.orig
 +++ builtin/gc.c
-@@ -243,7 +243,7 @@ static uint64_t total_ram(void)
+@@ -244,20 +244,13 @@ static uint64_t total_ram(void)
 
  if (!sysinfo(&si))
  return si.totalram;
 -#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM))
-+#elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM64))
- int64_t physical_memory;
- int mib[2];
- size_t length;
-@@ -252,9 +252,19 @@ static uint64_t total_ram(void)
- # if defined(HW_MEMSIZE)
- mib[1] = HW_MEMSIZE;
- # else
+- int64_t physical_memory;
+- int mib[2];
+- size_t length;
++#elif defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
++ long phys_pages, pagesize;
+
+- mib[0] = CTL_HW;
+-# if defined(HW_MEMSIZE)
+- mib[1] = HW_MEMSIZE;
+-# else
 - mib[1] = HW_PHYSMEM;
-+ mib[1] = HW_PHYSMEM64;
- # endif
- length = sizeof(int64_t);
-+ if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
-+ return physical_memory;
-+#elif defined(HAVE_BSD_SYSCTL) && defined(HW_PHYSMEM))
-+ int physical_memory;
-+ int mib[2];
-+ size_t length;
-+
-+ mib[0] = CTL_HW;
-+ mib[1] = HW_PHYSMEM;
-+ length = sizeof(int);
- if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
- return physical_memory;
+-# endif
+- length = sizeof(int64_t);
+- if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
+- return physical_memory;
++ phys_pages = sysconf(_SC_PHYS_PAGES);
++ pagesize = sysconf(_SC_PAGESIZE);
++ if (phys_pages != -1 && pagesize != -1)
++ return (uint64_t)phys_pages * (uint64_t)pagesize;
  #elif defined(GIT_WINDOWS_NATIVE)
+ MEMORYSTATUSEX memInfo;
+


--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Klemens Nanni-2
On Mon, Nov 11, 2019 at 09:06:59PM +0200, Jeremie Courreges-Anglas wrote:
> > If people like this approach I can try to push it upstream, along with
> > the ncpu diff.
Using sysconf(3) seems simple and sane.

OK kn

Reply | Threaded
Open this post in threaded view
|

Re: devel/git: cpu and memory tracking, LIB_DEPENDS

Benoit Lecocq-4
In reply to this post by Jeremie Courreges-Anglas-2


On 14/11/2019 21:52, Klemens Nanni wrote:
> On Mon, Nov 11, 2019 at 09:06:59PM +0200, Jeremie Courreges-Anglas wrote:
>>> If people like this approach I can try to push it upstream, along with
>>> the ncpu diff.
> Using sysconf(3) seems simple and sane.
>
> OK kn
>

Yes, ok benoit@