bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Make task_info.h structs more portable


From: Flavio Cruz
Subject: Re: [PATCH] Make task_info.h structs more portable
Date: Mon, 5 Dec 2022 01:22:14 -0500

Changed vm_size_t to rpc_size_t so that both userland and kernel agree
on the same size. Also changed the denominator for the maximum struct
sizes to be integer_t to match the other declarations.

Introduced long_natural_t and rpc_long_natural_t to represent large
counters. Replaced rpc_unsigned_long with rpc_long_natural_t.
---
On Sat, Dec 03, 2022 at 08:17:55PM +0100, Samuel Thibault wrote:
Flavio Cruz, le jeu. 01 déc. 2022 00:10:19 -0500, a ecrit:
On Wed, Nov 30, 2022 at 10:21:28PM +0100, Samuel Thibault wrote:
> Flavio Cruz, le mer. 30 nov. 2022 02:10:16 -0500, a ecrit:
> >  @table @code
> > -@item natural_t faults
> > +@item unsigned int faults
> >  number of page faults
> >
> > -@item natural_t zero_fills
> > +@item unsigned int zero_fills
> >  number of zero fill pages
> >
> > -@item natural_t reactivations
> > +@item unsigned int reactivations
> >  number of reactivated pages
> >
> > -@item natural_t pageins
> > +@item unsigned int pageins
> >  number of actual pageins
> >
> > -@item natural_t cow_faults
> > +@item unsigned int cow_faults
> >  number of copy-on-write faults
> >
> > -@item natural_t messages_sent
> > +@item unsigned int messages_sent
> >  number of messages sent
> >
> > -@item natural_t messages_received
> > +@item unsigned int messages_received
>
> It would be more convenient for these to be 64bit on 64bit systems?
>
> Of course 64bit can overflow as well, but at least it makes it easy to
> write simple programs to get it and have very little probability of
> overflowing.

I think that should fine. Would it be ok to define a new type called
rpc_unsigned_long to define those counters? It would be 32 bits or 64 bits
long depending on the userland.

Mmm, I'm not sure about the name.

natural_t and integer_t were supposed to be "the size of a register",
and castable to a pointer. It seems that xnu dropped the pointer part,
just like you seem to imply here, i.e. natural_t and integer_t would
remain "just an integer that's not too small".

It seems they kept events_info using integer_t rather than a large
number. But I'd still rather upgrade to 64bit. I'd also say we'd rather
keep coherent with the historical natural/integer names.

I would thus say we could go with long_natural_t and long_integer_t,
and be very explicit about the status of natural/integer_t and
long_natural/integer_t, something like:

- natural/integer_t are integer types which are reasonably small, making
 them suitable for most counters, while possibly be subject to
 overflows.

- long_natural/integer_t are integer types which are possibly larger than
 natural/integer, whenever the processor supports it, making them less
 subject to overflows.

And explicit that all of these types are not related to the size of
pointers, offsets, sizes.

Samuel

I like your proposal of keeping the historical integer_t/natural_t naming. I've reworked the patch so that we introduce long_natural_t and rpc_long_natural_t (to keep it compatible with a 32 bit proc server). Since I had already introduced rpc_unsigned_long before, I replaced it with rpc_long_natural_t. At some point, we should look over the existing kernel data structures to see if it makes sense to replace natural_t with long_natural_t (at least kern/thread.h should be updated for struct task).

I also updated the comment for natural_t to say that indeed it is not meant to represent a pointer anymore. I don't think we have any code left that makes that assumption after I removed some of the existing dead code in previous patches.

Thanks

diff --git a/doc/mach.texi b/doc/mach.texi
index fecd097b..86a557cb 100644
--- a/doc/mach.texi
+++ b/doc/mach.texi
@@ -5011,10 +5011,10 @@ suspend count for task
 @item integer_t base_priority
 base scheduling priority
-@item vm_size_t virtual_size
+@item rpc_vm_size_t virtual_size
 number of virtual pages
-@item vm_size_t resident_size
+@item rpc_vm_size_t resident_size
 number of resident pages
@item time_value_t user_time
@@ -5041,25 +5041,25 @@ provided it as the @var{task_info} parameter for the
 following members:
@table @code
-@item natural_t faults
+@item rpc_long_natural_t faults
 number of page faults
-@item natural_t zero_fills
+@item rpc_long_natural_t zero_fills
 number of zero fill pages
-@item natural_t reactivations
+@item rpc_long_natural_t reactivations
 number of reactivated pages
-@item natural_t pageins
+@item rpc_long_natural_t pageins
 number of actual pageins
-@item natural_t cow_faults
+@item rpc_long_natural_t cow_faults
 number of copy-on-write faults
-@item natural_t messages_sent
+@item rpc_long_natural_t messages_sent
 number of messages sent
-@item natural_t messages_received
+@item rpc_long_natural_t messages_received
 number of messages received
 @end table
 @end deftp
diff --git a/i386/include/mach/i386/machine_types.defs 
b/i386/include/mach/i386/machine_types.defs
index 0e94999b..f3de1e44 100755
--- a/i386/include/mach/i386/machine_types.defs
+++ b/i386/include/mach/i386/machine_types.defs
@@ -38,14 +38,10 @@
/*
  * A natural_t is the type for the native
- * integer type, e.g. 32 or 64 or.. whatever
- * register size the machine has.  Unsigned, it is
- * used for entities that might be either
- * unsigned integers or pointers, and for
- * type-casting between the two.
- * For instance, the IPC system represents
- * a port in user space as an integer and
- * in kernel space as a pointer.
+ * unsigned integer type, usually 32 bits. It is suitable for
+ * most counters with a small chance of overflow.
+ * While historically natural_t was meant to be the same size
+ * as a pointer, that is not the case anymore.
  */
 type natural_t = uint32_t;
@@ -59,17 +55,24 @@ type natural_t = uint32_t;
 type integer_t = int32_t;
/*
- * unsigned long for kernel <-> userland interfaces size depends on the 
architecture
- * of both kernel and userland.
+ * A long_natural_t is a possibly larger unsigned integer type than natural_t.
+ * Should be used instead of natural_t when we want the data to be less subject
+ * to overflows.
  */
-#if defined(KERNEL) && defined(USER32)
-type rpc_unsigned_long = uint32_t;
-#else /* KERNEL and USER32 */
 #if defined(__x86_64__)
-type rpc_unsigned_long = uint64_t;
-#else /* __x86_64__ */
-type rpc_unsigned_long = uint32_t;
+type long_natural_t = uint64_t;
+#else
+type long_natural_t = uint32_t;
 #endif /* __x86_64__ */
+
+/*
+ * long_natural_t for kernel <-> userland interfaces as the size depends on the
+ * architecture of both kernel and userland.
+ */
+#if defined(KERNEL) && defined(USER32)
+type rpc_long_natural_t = uint32_t;
+#else /* KERNEL and USER32 */
+type rpc_long_natural_t = long_natural_t;
 #endif /* KERNEL_SERVER and USER32 */
/*
diff --git a/i386/include/mach/i386/vm_types.h 
b/i386/include/mach/i386/vm_types.h
index 9daaa15e..a50665c8 100644
--- a/i386/include/mach/i386/vm_types.h
+++ b/i386/include/mach/i386/vm_types.h
@@ -45,18 +45,11 @@
/*
  * A natural_t is the type for the native
- * integer type, e.g. 32 or 64 or.. whatever
- * register size the machine has.  Unsigned, it is
- * used for entities that might be either
- * unsigned integers or pointers, and for
- * type-casting between the two.
- * For instance, the IPC system represents
- * a port in user space as an integer and
- * in kernel space as a pointer.
+ * unsigned integer type, usually 32 bits. It is suitable for
+ * most counters with a small chance of overflow.
+ * While historically natural_t was meant to be the same
+ * as a pointer, that is not the case here.
  */
-#ifdef __x86_64__
-// unsigned long ?
-#endif
 typedef unsigned int   natural_t;
/*
@@ -68,6 +61,13 @@ typedef unsigned int natural_t;
  */
 typedef int            integer_t;
+/*
+ * A long_natural_t is a possibly larger unsigned integer type than natural_t.
+ * Should be used instead of natural_t when we want the data to be less subject
+ * to overflows.
+ */
+typedef unsigned long long_natural_t;
+
 /*
  * A vm_offset_t is a type-neutral pointer,
  * e.g. an offset into a virtual memory space.
@@ -116,14 +116,14 @@ static inline __mach_uint32_t 
convert_vm_to_user(__mach_uint64_t kaddr)
     assert(kaddr <= 0xFFFFFFFF);
     return (__mach_uint32_t)kaddr;
 }
-typedef __mach_uint32_t rpc_unsigned_long;
+typedef __mach_uint32_t rpc_long_natural_t;
 #else /* MACH_KERNEL */
 typedef vm_offset_t    rpc_vm_address_t;
 typedef vm_offset_t    rpc_vm_offset_t;
 typedef vm_size_t      rpc_vm_size_t;
 #define convert_vm_to_user null_conversion
 #define convert_vm_from_user null_conversion
-typedef unsigned long rpc_unsigned_long;
+typedef long_natural_t rpc_long_natural_t;
 #endif /* MACH_KERNEL */
#endif /* __ASSEMBLER__ */
diff --git a/include/mach/task_info.h b/include/mach/task_info.h
index 5607178c..3aaa7cd6 100644
--- a/include/mach/task_info.h
+++ b/include/mach/task_info.h
@@ -54,8 +54,8 @@ typedef       integer_t       task_info_data_t[TASK_INFO_MAX];
 struct task_basic_info {
        integer_t       suspend_count;  /* suspend count for task */
        integer_t       base_priority;  /* base scheduling priority */
-       vm_size_t       virtual_size;   /* number of virtual pages */
-       vm_size_t       resident_size;  /* number of resident pages */
+       rpc_vm_size_t   virtual_size;   /* number of virtual pages */
+       rpc_vm_size_t   resident_size;  /* number of resident pages */
        time_value_t    user_time;      /* total user run time for
                                           terminated threads */
        time_value_t    system_time;    /* total system run time for
@@ -66,24 +66,24 @@ struct task_basic_info {
 typedef struct task_basic_info         task_basic_info_data_t;
 typedef struct task_basic_info         *task_basic_info_t;
 #define        TASK_BASIC_INFO_COUNT   \
-               (sizeof(task_basic_info_data_t) / sizeof(natural_t))
+               (sizeof(task_basic_info_data_t) / sizeof(integer_t))
#define TASK_EVENTS_INFO 2 /* various event counts */ struct task_events_info {
-       natural_t               faults;         /* number of page faults */
-       natural_t               zero_fills;     /* number of zero fill pages */
-       natural_t               reactivations;  /* number of reactivated pages 
*/
-       natural_t               pageins;        /* number of actual pageins */
-       natural_t               cow_faults;     /* number of copy-on-write 
faults */
-       natural_t               messages_sent;  /* number of messages sent */
-       natural_t               messages_received; /* number of messages 
received */
+       rpc_long_natural_t      faults;         /* number of page faults */
+       rpc_long_natural_t      zero_fills;     /* number of zero fill pages */
+       rpc_long_natural_t      reactivations;  /* number of reactivated pages 
*/
+       rpc_long_natural_t      pageins;        /* number of actual pageins */
+       rpc_long_natural_t      cow_faults;     /* number of copy-on-write 
faults */
+       rpc_long_natural_t      messages_sent;  /* number of messages sent */
+       rpc_long_natural_t      messages_received; /* number of messages 
received */
 };
 typedef struct task_events_info                task_events_info_data_t;
 typedef struct task_events_info                *task_events_info_t;
 #define        TASK_EVENTS_INFO_COUNT  \
-               (sizeof(task_events_info_data_t) / sizeof(natural_t))
+               (sizeof(task_events_info_data_t) / sizeof(integer_t))
#define TASK_THREAD_TIMES_INFO 3 /* total times for live threads -
                                           only accurate if suspended */
@@ -98,7 +98,7 @@ struct task_thread_times_info {
 typedef struct task_thread_times_info  task_thread_times_info_data_t;
 typedef struct task_thread_times_info  *task_thread_times_info_t;
 #define        TASK_THREAD_TIMES_INFO_COUNT    \
-               (sizeof(task_thread_times_info_data_t) / sizeof(natural_t))
+               (sizeof(task_thread_times_info_data_t) / sizeof(integer_t))
/*
  * Flavor definitions for task_ras_control
diff --git a/include/mach_debug/mach_debug_types.defs 
b/include/mach_debug/mach_debug_types.defs
index c138dc40..fd940384 100644
--- a/include/mach_debug/mach_debug_types.defs
+++ b/include/mach_debug/mach_debug_types.defs
@@ -42,11 +42,11 @@ type cache_info_t = struct {
    rpc_vm_size_t align;
    rpc_vm_size_t buf_size;
    rpc_vm_size_t slab_size;
-   rpc_unsigned_long bufs_per_slab;
-   rpc_unsigned_long nr_objs;
-   rpc_unsigned_long nr_bufs;
-   rpc_unsigned_long nr_slabs;
-   rpc_unsigned_long nr_free_slabs;
+   rpc_long_natural_t bufs_per_slab;
+   rpc_long_natural_t nr_objs;
+   rpc_long_natural_t nr_bufs;
+   rpc_long_natural_t nr_slabs;
+   rpc_long_natural_t nr_free_slabs;
    cache_name_t name;
 };
 type cache_info_array_t = array[] of cache_info_t;
diff --git a/include/mach_debug/slab_info.h b/include/mach_debug/slab_info.h
index 19a87307..0f6b5a2c 100644
--- a/include/mach_debug/slab_info.h
+++ b/include/mach_debug/slab_info.h
@@ -43,11 +43,11 @@ typedef struct cache_info {
        rpc_vm_size_t align;
        rpc_vm_size_t buf_size;
        rpc_vm_size_t slab_size;
-       rpc_unsigned_long bufs_per_slab;
-       rpc_unsigned_long nr_objs;
-       rpc_unsigned_long nr_bufs;
-       rpc_unsigned_long nr_slabs;
-       rpc_unsigned_long nr_free_slabs;
+       rpc_long_natural_t bufs_per_slab;
+       rpc_long_natural_t nr_objs;
+       rpc_long_natural_t nr_bufs;
+       rpc_long_natural_t nr_slabs;
+       rpc_long_natural_t nr_free_slabs;
        char name[CACHE_NAME_MAX_LEN];
 } cache_info_t;
--
2.37.2




reply via email to

[Prev in Thread] Current Thread [Next in Thread]