NetBSD Problem Report #57772

From www@netbsd.org  Thu Dec 14 16:43:14 2023
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id E0FAF1A9238
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 14 Dec 2023 16:43:13 +0000 (UTC)
Message-Id: <20231214164312.B39741A923C@mollari.NetBSD.org>
Date: Thu, 14 Dec 2023 16:43:12 +0000 (UTC)
From: tlaronde@kergis.com
Reply-To: tlaronde@kergis.com
To: gnats-bugs@NetBSD.org
Subject: DRMKMS (drm2/): incorrect memory compuation
X-Send-Pr-Version: www-1.0

>Number:         57772
>Category:       kern
>Synopsis:       DRMKMS (drm2/): incorrect memory compuation
>Confidential:   no
>Severity:       critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Dec 14 16:45:00 +0000 2023
>Last-Modified:  Sat Dec 16 07:50:02 +0000 2023
>Originator:     Thierry LARONDE
>Release:        Current
>Organization:
>Environment:
NetBSD cauchy.polynum.local 10.99.10 NetBSD 10.99.10 (CAUCHY) #4: Thu Dec 14 17:15:06 CET 2023  tlaronde@cauchy.polynum.local:/data/m/netbsd-10/sys/arch/amd64/compile/CAUCHY amd64
>Description:
Memory allocation is done based on values defined in a reduced Linux sysinfo(2) like struct.

But *high doesn't make sense in NetBSD and even on Linux it is not always used.

Furthermore, the current value assigned is a virtual address leading to such display:

[     4.193896] Zone  kernel: Available graphics memory: 9007199254113272 KiB
[     4.193896] Zone   dma32: Available graphics memory: 2097152 KiB

And the totalhigh value is used in amdgpu code where it can do, with such a value, no good.
>How-To-Repeat:

>Fix:

>Audit-Trail:
From: tlaronde@kergis.com
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/57772: DRMKMS [FIXES]
Date: Thu, 14 Dec 2023 17:49:00 +0100

 --k+baU34RwfPKpsZu
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline


 Correct the DRMKMS (drm2/) memory computation:

   - Linux *high mem is not always used (may be -1) and does not
     map to NetBSD mem usage: neutralize it by setting it to 0
     and don't use it for mem evaluation (consumer is amdgpu; the
     code perhaps better behave now);

   - Use uvm_availmem() for freeram, cached value for routine
     that may be called frequently; requiring update for sysinfo
     definition;

   - For allocation, always start with freeram (not totalram),
     setting a max of half the available (equal to Linux code
     behavior);

   - Restore Linux code behavior for dma32: zone can not be
     more than 32bits or freeram (the min) and take at max half.

     This corrects such display due to spurious totalhigh value:

 [     4.202485] Zone  kernel: Available graphics memory: 9007199254113272 KiB
 [     4.202485] Zone   dma32: Available graphics memory: 2097152 KiB

     giving:

 [     4.203937] Zone  kernel: Available graphics memory: 3962884 KiB
 [     4.203937] Zone   dma32: Available graphics memory: 2097152 KiB

 -- 
         Thierry Laronde <tlaronde +AT+ kergis +dot+ com>
                      http://www.kergis.com/
                     http://kertex.kergis.com/
 Key fingerprint = 0FF7 E906 FBAF FE95 FD89  250D 52B1 AE95 6006 F40C

 --k+baU34RwfPKpsZu
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename=diff

 diff --git a/sys/external/bsd/drm2/dist/drm/ttm/ttm_memory.c b/sys/external/bsd/drm2/dist/drm/ttm/ttm_memory.c
 index 05eb3e8feaf8..4e141f93e953 100644
 --- a/sys/external/bsd/drm2/dist/drm/ttm/ttm_memory.c
 +++ b/sys/external/bsd/drm2/dist/drm/ttm/ttm_memory.c
 @@ -317,11 +317,12 @@ static int ttm_mem_init_kernel_zone(struct ttm_mem_global *glob,
  	if (unlikely(!zone))
  		return -ENOMEM;

 -	mem = si->totalram - si->totalhigh;
 +	mem = si->freeram;
  	mem *= si->mem_unit;

  	zone->name = "kernel";
  	zone->zone_mem = mem;
 +	/* use at most half the available */
  	zone->max_mem = mem >> 1;
  	zone->emer_mem = (mem >> 1) + (mem >> 2);
  	zone->swap_limit = zone->max_mem - (mem >> 3);
 @@ -393,27 +394,17 @@ static int ttm_mem_init_dma32_zone(struct ttm_mem_global *glob,
  	if (unlikely(!zone))
  		return -ENOMEM;

 -	mem = si->totalram;
 -	mem *= si->mem_unit;
 -
 -	/**
 -	 * No special dma32 zone needed.
 -	 */
 -
 -	if (mem <= ((uint64_t) 1ULL << 32)) {
 -		kfree(zone);
 -		return 0;
 -	}
 -
  	/*
 -	 * Limit max dma32 memory to 4GB for now
 -	 * until we can figure out how big this
 -	 * zone really is.
 +	 * Limit max dma32 memory to half the available but this
 +	 * has to be less than 4GB for the zone.
  	 */
 +	mem = si->freeram;
 +	mem *= si->mem_unit;
 +	mem = (mem <= ((uint64_t) 1ULL << 32))? mem : 1ULL << 32;

 -	mem = ((uint64_t) 1ULL << 32);
  	zone->name = "dma32";
  	zone->zone_mem = mem;
 +	/* use at most half the available */
  	zone->max_mem = mem >> 1;
  	zone->emer_mem = (mem >> 1) + (mem >> 2);
  	zone->swap_limit = zone->max_mem - (mem >> 3);
 diff --git a/sys/external/bsd/drm2/include/linux/mm.h b/sys/external/bsd/drm2/include/linux/mm.h
 index fc71c98e46eb..e467f0ede6e2 100644
 --- a/sys/external/bsd/drm2/include/linux/mm.h
 +++ b/sys/external/bsd/drm2/include/linux/mm.h
 @@ -53,9 +53,16 @@ struct file;

  #define	untagged_addr(x)	(x)

 +/*
 + * All values (except mem_unit of course) are pages. Only used or
 + * making some sense on NetBSD members are filled: *high do not make
 + * sense (these are even not always enabled in Linux). totalhigh is
 + * set to 0 to neutralize the use, even in conditional HIGHMEM blocks.
 + */
  struct sysinfo {
  	unsigned long totalram;
 -	unsigned long totalhigh;
 +	unsigned long freeram;
 +	unsigned long totalhigh;	/* does not make sense here */
  	uint32_t mem_unit;
  };

 @@ -64,7 +71,8 @@ si_meminfo(struct sysinfo *si)
  {

  	si->totalram = uvmexp.npages;
 -	si->totalhigh = kernel_map->size >> PAGE_SHIFT;
 +	si->freeram = uvm_availmem(false);	/* require value */
 +	si->totalhigh = 0;	/* just to keep code as is; neutralize */
  	si->mem_unit = PAGE_SIZE;
  	/* XXX Fill in more as needed.  */
  }
 @@ -72,9 +80,7 @@ si_meminfo(struct sysinfo *si)
  static inline size_t
  si_mem_available(void)
  {
 -
 -	/* XXX ? */
 -	return uvmexp.free;
 +	return uvm_availmem(true);	/* use cached value */
  }

  static inline unsigned long

 --k+baU34RwfPKpsZu--

From: tlaronde@kergis.com
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/57772: DRMKMS [FIXES]
Date: Sat, 16 Dec 2023 08:47:16 +0100

 I got it wrong: on some arches (for example x68k) the kernel pages can
 not be put anywhere in ram but only on low addresses thus limiting
 de facto the amount of memory available for kernel.

 But such limits can exist for dma mapping too (and differently).

 So correct the comment. The fix avoids publishing crazy numbers, and
 since this is not allocation but fixing an upper limit on what _could_
 be allocated, this can do for now.

 But this is too simplistic. This is MD and should be handled with more
 care.

 diff --git a/sys/external/bsd/drm2/include/linux/mm.h b/sys/external/bsd/drm2/include/linux/mm.h
 index e467f0ede6e2..79a958d10852 100644
 --- a/sys/external/bsd/drm2/include/linux/mm.h
 +++ b/sys/external/bsd/drm2/include/linux/mm.h
 @@ -54,15 +54,29 @@ struct file;
  #define	untagged_addr(x)	(x)

  /*
 - * All values (except mem_unit of course) are pages. Only used or
 - * making some sense on NetBSD members are filled: *high do not make
 - * sense (these are even not always enabled in Linux). totalhigh is
 - * set to 0 to neutralize the use, even in conditional HIGHMEM blocks.
 + * All values (except mem_unit of course) are pages. Only NetBSD
 + * used members or members appearing in the code (but not used here)
 + * are declared.
 + */
 +
 +/* XXX
 + *
 + * High mem have some meaning in some arches where the kernel
 + * pages can not be put anywhere in ram but have to appear in a
 + * limited range (low range).
 + * But on some arches, dma can not be map anywhere either.
 + * These are two different things, to be set M.D.
 + * For the moment, we make it simple: let's assume that the whole ram
 + * can be used for kernel and dma. This is used only to define what
 + * _could_ be used. Real * allocation is another subject.
 + *
 + * For the moment, totalhigh is set to 0 in order to neutralize the use, 
 + * even in conditional HIGHMEM blocks.
   */
  struct sysinfo {
  	unsigned long totalram;
  	unsigned long freeram;
 -	unsigned long totalhigh;	/* does not make sense here */
 +	unsigned long totalhigh;	/* XXX arch dependent */
  	uint32_t mem_unit;
  };

 -- 
         Thierry Laronde <tlaronde +AT+ kergis +dot+ com>
                      http://www.kergis.com/
                     http://kertex.kergis.com/
 Key fingerprint = 0FF7 E906 FBAF FE95 FD89  250D 52B1 AE95 6006 F40C

Home	PR Database Search