[PATCH v2 0/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[PATCH v2 0/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

PAGE_SIZE is not defined in all build configurations, therefore,
use the posix call sysconf (_SC_PAGESIZE)
build system configured with --enable-build-warnings --enable-gdb-build-warnings --enable-werror
and the build was not raising any warning or error

build_intel/gdb$ make
  CXX    nat/linux-btrace.o
  GEN    init.c
  CXXLD  gdb


Zied Guermazi (1):
  get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

 gdb/nat/linux-btrace.c | 44 ++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH v2 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

PAGE_SIZE is not defined in all build configurations, e.g cross compiling
for aarch64 GNU/Linux machine. This patch gets the value at runtime using 
posix call sysconf (_SC_PAGESIZE)
---
 gdb/nat/linux-btrace.c | 44 ++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index b0d6dcd7cf1..18b81f7e750 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -486,9 +486,13 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
   if (fd.get () < 0)
     diagnose_perf_event_open_fail ();
 
+  long page_size = sysconf (_SC_PAGESIZE);
+  if (page_size == -1)
+    error (_("Failed to get system page size: %s"), safe_strerror (errno));
+
   /* Convert the requested size in bytes to pages (rounding up).  */
-  pages = ((size_t) conf->size / PAGE_SIZE
-	   + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1));
+  pages = ((size_t) conf->size / page_size
+	   + ((conf->size % page_size) == 0 ? 0 : 1));
   /* We need at least one page.  */
   if (pages == 0)
     pages = 1;
@@ -507,17 +511,17 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
       size_t length;
       __u64 data_size;
 
-      data_size = (__u64) pages * PAGE_SIZE;
+      data_size = (__u64) pages * (__u64) page_size;
 
       /* Don't ask for more than we can represent in the configuration.  */
       if ((__u64) UINT_MAX < data_size)
 	continue;
 
       size = (size_t) data_size;
-      length = size + PAGE_SIZE;
+      length = size + (size_t) page_size;
 
       /* Check for overflows.  */
-      if ((__u64) length != data_size + PAGE_SIZE)
+      if ((__u64) length != data_size + (__u64) page_size)
 	continue;
 
       errno = 0;
@@ -532,7 +536,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
 
   struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
     data.get ();
-  data_offset = PAGE_SIZE;
+  data_offset = (__u64) page_size;
 
 #if defined (PERF_ATTR_SIZE_VER5)
   if (offsetof (struct perf_event_mmap_page, data_size) <= header->size)
@@ -630,8 +634,12 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
     diagnose_perf_event_open_fail ();
 
   /* Allocate the configuration page. */
-  scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
-		    fd.get (), 0);
+  long page_size = sysconf (_SC_PAGESIZE);
+    if (page_size == -1)
+    error (_("Failed to get system page size: %s"), safe_strerror (errno));
+
+  scoped_mmap data (nullptr, (size_t) page_size, PROT_READ | PROT_WRITE,
+		     MAP_SHARED, fd.get (), 0);
   if (data.get () == MAP_FAILED)
     error (_("Failed to map trace user page: %s."), safe_strerror (errno));
 
@@ -641,8 +649,8 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   header->aux_offset = header->data_offset + header->data_size;
 
   /* Convert the requested size in bytes to pages (rounding up).  */
-  pages = ((size_t) conf->size / PAGE_SIZE
-	   + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1));
+  pages = ((size_t) conf->size / page_size
+	   + ((conf->size % page_size) == 0 ? 0 : 1));
   /* We need at least one page.  */
   if (pages == 0)
     pages = 1;
@@ -661,7 +669,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
       size_t length;
       __u64 data_size;
 
-      data_size = (__u64) pages * PAGE_SIZE;
+      data_size = (__u64) pages * (__u64) page_size;
 
       /* Don't ask for more than we can represent in the configuration.  */
       if ((__u64) UINT_MAX < data_size)
@@ -732,7 +740,11 @@ linux_enable_btrace (ptid_t ptid, const struct btrace_config *conf)
 static enum btrace_error
 linux_disable_bts (struct btrace_tinfo_bts *tinfo)
 {
-  munmap((void *) tinfo->header, tinfo->bts.size + PAGE_SIZE);
+  long page_size = sysconf (_SC_PAGESIZE);
+  if (page_size == -1)
+    error (_("Failed to get system page size: %s"), safe_strerror (errno));
+
+  munmap ((void *) tinfo->header, (size_t) (tinfo->bts.size + page_size));
   close (tinfo->file);
 
   return BTRACE_ERR_NONE;
@@ -743,8 +755,12 @@ linux_disable_bts (struct btrace_tinfo_bts *tinfo)
 static enum btrace_error
 linux_disable_pt (struct btrace_tinfo_pt *tinfo)
 {
-  munmap((void *) tinfo->pt.mem, tinfo->pt.size);
-  munmap((void *) tinfo->header, PAGE_SIZE);
+  long page_size = sysconf (_SC_PAGESIZE);
+  if (page_size == -1)
+    error (_("Failed to get system page size: %s"), safe_strerror (errno));
+
+  munmap ((void *) tinfo->pt.mem, tinfo->pt.size);
+  munmap ((void *) tinfo->header, (size_t) page_size);
   close (tinfo->file);
 
   return BTRACE_ERR_NONE;
-- 
2.25.1

Re: [PATCH v2 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Tom Tromey]

>>>>> Zied Guermazi <zied.guermazi@trande.de> writes:

>  static enum btrace_error
>  linux_disable_bts (struct btrace_tinfo_bts *tinfo)
>  {
> -  munmap((void *) tinfo->header, tinfo->bts.size + PAGE_SIZE);
> +  long page_size = sysconf (_SC_PAGESIZE);
> +  if (page_size == -1)
> +    error (_("Failed to get system page size: %s"), safe_strerror (errno));
> +
> +  munmap ((void *) tinfo->header, (size_t) (tinfo->bts.size + page_size));

It seems a little strange to me to call sysconf again here, rather than
just recording the size that was used when mmapping.

An even better option would be to avoid the release calls like this one:

  pt->header = (struct perf_event_mmap_page *) data.release ();

... and let this be managed by the scoped_mmap object itself.  However
that might be a lot more invasive.

Tom

Re: [PATCH v2 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Pedro Alves]

On 2022-04-25 11:23, Zied Guermazi wrote:
> +  if (page_size == -1)
> +    error (_("Failed to get system page size: %s"), safe_strerror (errno));

This is equivalent to:

  if (page_size == -1)
    perror_with_name (_("Failed to get system page size"));


> @@ -630,8 +634,12 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
...
> +  long page_size = sysconf (_SC_PAGESIZE);
> +    if (page_size == -1)
> +    error (_("Failed to get system page size: %s"), safe_strerror (errno));

Indentation of that "if" seems off here.

Also, that pattern appears a number of times in the patch, all in the same file.
You could add a static function like so:

/* Suitable intro comment.  */

 static long
 system_page_size ()
 {
   long page_size = sysconf (_SC_PAGESIZE);
   if (page_size == -1)
     perror_with_name (_("Failed to get system page size: %s"));
   return page_size;
 }

and then system_page_size throughout without having to worry about checking
the return value for error everywhere.

Re: [PATCH v2 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

hi Pedro, 
thanks for your feedback. See below for the answers to your comments 
Am 2022-04-26 21:05, schrieb Pedro Alves: 

> On 2022-04-25 11:23, Zied Guermazi wrote: 
> 
>> +  if (page_size == -1)
>> +    error (_("Failed to get system page size: %s"), safe_strerror (errno));
> 
> This is equivalent to:
> 
> if (page_size == -1)
> perror_with_name (_("Failed to get system page size"));
 [Zied] will be changed as suggested in V3 of the patch

@@ -630,8 +634,12 @@ linux_enable_pt (ptid_t ptid, const struct
btrace_config_pt *conf)...+  long page_size = sysconf (_SC_PAGESIZE);
+    if (page_size == -1)
+    error (_("Failed to get system page size: %s"), safe_strerror
(errno));
Indentation of that "if" seems off here.

Also, that pattern appears a number of times in the patch, all in the
same file.
You could add a static function like so:

/* Suitable intro comment.  */

 static long
 system_page_size ()
 {
   long page_size = sysconf (_SC_PAGESIZE);
   if (page_size == -1)
     perror_with_name (_("Failed to get system page size: %s"));
   return page_size;
 }

and then system_page_size throughout without having to worry about
checking
the return value for error everywhere. 

[Zied] will be changed in V3 as suggested. 

Kind Regards 
Zied Guermazi

Re: [PATCH v2 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

hi Tom,
thanks for your feedback. The reason for the decision to not record the 
page size and reuse it, is that sysconf (_SC_PAGESIZE) gets its value 
from a system constant (see long int __sysconf (int name) in 
glibc/sysdeps/posix/sysconf.c and int __getpagesize (void)  in 
glibc/sysdeps/unix/sysv/linux/getpagesize.c and size_t _dl_pagesize = 
EXEC_PAGESIZE; in glibc/elf/dl-support.c) without much processing, so 
from my point of view, there is no need of replicating it.

for the second point: "let this be managed by the scoped_mmap object 
itself", I recommend involving Markus as the original submitter of this 
code in this discussion.

Kind Regards
Zied Guermazi

Am 2022-04-26 20:13, schrieb Tom Tromey:
>>>>>> Zied Guermazi <zied.guermazi@trande.de> writes:
> 
>>  static enum btrace_error
>>  linux_disable_bts (struct btrace_tinfo_bts *tinfo)
>>  {
>> -  munmap((void *) tinfo->header, tinfo->bts.size + PAGE_SIZE);
>> +  long page_size = sysconf (_SC_PAGESIZE);
>> +  if (page_size == -1)
>> +    error (_("Failed to get system page size: %s"), safe_strerror 
>> (errno));
>> +
>> +  munmap ((void *) tinfo->header, (size_t) (tinfo->bts.size + 
>> page_size));
> 
> It seems a little strange to me to call sysconf again here, rather than
> just recording the size that was used when mmapping.
> 
> An even better option would be to avoid the release calls like this 
> one:
> 
>   pt->header = (struct perf_event_mmap_page *) data.release ();
> 
> ... and let this be managed by the scoped_mmap object itself.  However
> that might be a lot more invasive.
> 
> Tom