Re: systat(1) counter overflow
On Mon, Aug 30, 2021 at 11:11:55AM +0200, Martin Pieuchot wrote: > On 13/07/21(Tue) 00:55, Anindya Mukherjee wrote: > > On Sat, Jul 03, 2021 at 11:20:42AM +0100, Stuart Henderson wrote: > > > On 2021/07/03 01:09, Anindya Mukherjee wrote: > > > > Thanks for the discussion. This has been very illuminating. I have been > > > > digging > > > > around in /usr/src/ and ignoring the atomic architectures (where I got > > > > stuck) it > > > > looks like it should be possible to use uint64_t everywhere. I'm > > > > playing with > > > > some changes on my machine to see if I can get at least systat(1) and > > > > vmstat(8) > > > > to work with uint64_t. The ddb printer (uvmexp_print)is another > > > > consumer. > > > > > > > > If it works in the base system then ideally every relevant port should > > > > be > > > > updated to be consistent. That is indeed quite a big change; more than I > > > > realised so thanks for setting me straight on that. > > > > > > We have coped with bigger changes in structs like this before, > > > it didn't used to be too difficult, but that was before go... > > > > > > > Hi, > > > > I have been running for a week with the following diff. This is just a POC > > and > > hence there are a few ugly hacks. So far top(1), systat(1), and vmstat(8) > > seem > > to be happy. I haven't hit the 32-bit overflow point for any counters yet > > but > > the counts look right. I have completely ignored ports, but it looks like > > the > > base system can run with this change. This was mostly to satisfy my > > curiosity. > > Thanks for your work. > > There's no guarantee that 64bit value can be updated atomically on 32bit > architecture, so we can't follow this road. > > It seems to me that this issue requires a bit more investigation. The > existing "struct uvmexp" contains multiple fields with multiple purposes: > > 1. constants (pagesize, pagemask, pageshift, ...) > > 2. counters frequently updated, only incremented and only used >for accounting purposes (faults, pageinsm fltnoram, ...) > > 3. counters rarely updated, incremented and decremented >(swpgonly, nswapdev, ...) > > 4. global variables, that are incremented/decremented and used for >making decisions (wired, free, zeropages...) > > I don't believe all of them need to be of type uint64_t in the kernel. Agreed. In my test diff I promoted everything because of the way systat(1)'s uvm module is implemented currently, namely all uvmexp members are assumed to be of the same type. Ideally I would only like to change the stat and fault counters, which increase monotonically. The uvm module can be accordingly updated. If this is done we can leave swpgonly untouched and hence remove the 64-bit atomic functions from my diff. However, this doesn't solve the issue with atomic increments because in certain architectures some of the monotonic counters use atomic increments. An example: /usr/src/sys/arch/mips64/mips64/trap.c|184| atomic_inc_int(); I'm not sure how to get around this; traps specifically is a counter which overflows quickly. Maybe if the architecture is 64 bit and it uses atomic increment as above then we can use a 64-bit atomic function in the arch specific code? I don't fully understand the details here, but it seems that some of the counters are maintained by the kernel as 64-bit variables internally. These get converted during a sysctl(8) call in uvmexp_read and are being cast to int currently. Along with the counters which are only incremented using ++, these are the easiest to convert to 64-bit. > > It's also not clear to me if keeping "struct uvmexp" as it is but > bumping the size to 64bit is the best way forward. Did you look at > other userland consumer of "struct uvmexp"? Which fields do they care > about? If it is the way forward we could simply use a different layout > in the kernel and do the conversion during the syscall. I searched for usages of the stat and fault counters in /usr/src (again ignoring ports) and the only consumers in /usr/src/usr.bin are systat(1), vmstat(8), iostat(8), and top(1). The rest of the references in /usr/src are mostly from code which increments these counters directly or converts them in uvmexp_read. The function uvmexp_print in uvm_meter.c is used by ddb(4) to print these values. Regards, Anindya > > > ? usr.bin/systat/vim_session > > Index: sys/arch/amd64/include/atomic.h > > === > > RCS file: /cvs/src/sys/arch/amd64/include/atomic
Re: systat(1) iostat cumulative mode scaling issue
Many thanks! Anindya On Sat, Aug 14, 2021 at 08:22:45AM -0600, Todd C. Miller wrote: > On Wed, 28 Jul 2021 12:13:23 -0700, Anindya Mukherjee wrote: > > > While running systat(1)'s iostat view in cumulative mode (activated by > > pressi > > ng > > 'b') I noticed that it divides the results by the refresh interval. This is > > because the cumulative mode is implemented by setting last = 0, and then it > > d > > oes > > (current - last) / etime, thereby giving wrong cumulative results. This can > > b > > e > > verified by comparing with `iostat -dI`. The attached diff fixes this. > > Committed, thanks. Sorry for the delay. > > - todd
Re: systat(1) iostat cumulative mode scaling issue
ping
On Fri, Aug 06, 2021 at 02:34:07PM -0700, Anindya Mukherjee wrote:
> ping
>
> > Hi,
> >
> > While running systat(1)'s iostat view in cumulative mode (activated by
> > pressing
> > 'b') I noticed that it divides the results by the refresh interval. This is
> > because the cumulative mode is implemented by setting last = 0, and then it
> > does
> > (current - last) / etime, thereby giving wrong cumulative results. This can
> > be
> > verified by comparing with `iostat -dI`. The attached diff fixes this.
> >
> > ok?
> >
> > Index: usr.bin/systat/iostat.c
> > ===
> > RCS file: /cvs/src/usr.bin/systat/iostat.c,v
> > retrieving revision 1.49
> > diff -u -p -r1.49 iostat.c
> > --- usr.bin/systat/iostat.c 28 Jun 2019 13:35:04 - 1.49
> > +++ usr.bin/systat/iostat.c 28 Jul 2021 19:04:41 -
> > @@ -167,9 +167,12 @@ void
> > print_io(void)
> > {
> > int n, count = 0;
> > -
> > int curr;
> > - etime = naptime;
> > +
> > + if (state == BOOT)
> > + etime = 1.0;
> > + else
> > + etime = naptime;
> >
> > /* XXX engine internals: save and restore curr_line for bcache */
> > curr = curr_line;
Re: systat(1) iostat cumulative mode scaling issue
ping
> Hi,
>
> While running systat(1)'s iostat view in cumulative mode (activated by
> pressing
> 'b') I noticed that it divides the results by the refresh interval. This is
> because the cumulative mode is implemented by setting last = 0, and then it
> does
> (current - last) / etime, thereby giving wrong cumulative results. This can be
> verified by comparing with `iostat -dI`. The attached diff fixes this.
>
> ok?
>
> Index: usr.bin/systat/iostat.c
> ===
> RCS file: /cvs/src/usr.bin/systat/iostat.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 iostat.c
> --- usr.bin/systat/iostat.c 28 Jun 2019 13:35:04 - 1.49
> +++ usr.bin/systat/iostat.c 28 Jul 2021 19:04:41 -
> @@ -167,9 +167,12 @@ void
> print_io(void)
> {
> int n, count = 0;
> -
> int curr;
> - etime = naptime;
> +
> + if (state == BOOT)
> + etime = 1.0;
> + else
> + etime = naptime;
>
> /* XXX engine internals: save and restore curr_line for bcache */
> curr = curr_line;
systat(1) iostat cumulative mode scaling issue
Hi,
While running systat(1)'s iostat view in cumulative mode (activated by pressing
'b') I noticed that it divides the results by the refresh interval. This is
because the cumulative mode is implemented by setting last = 0, and then it does
(current - last) / etime, thereby giving wrong cumulative results. This can be
verified by comparing with `iostat -dI`. The attached diff fixes this.
ok?
Index: usr.bin/systat/iostat.c
===
RCS file: /cvs/src/usr.bin/systat/iostat.c,v
retrieving revision 1.49
diff -u -p -r1.49 iostat.c
--- usr.bin/systat/iostat.c 28 Jun 2019 13:35:04 - 1.49
+++ usr.bin/systat/iostat.c 28 Jul 2021 19:04:41 -
@@ -167,9 +167,12 @@ void
print_io(void)
{
int n, count = 0;
-
int curr;
- etime = naptime;
+
+ if (state == BOOT)
+ etime = 1.0;
+ else
+ etime = naptime;
/* XXX engine internals: save and restore curr_line for bcache */
curr = curr_line;
Re: systat(1) counter overflow
Hi, just a quick update. After about two weeks, various counters have exceeded the 32 bit int limit, and haven't wrapped. There might eventually be some issues with the field width but that will take a long time to reach. Regards, Anindya On Sat, Jul 03, 2021 at 11:20:42AM +0100, Stuart Henderson wrote: > On 2021/07/03 01:09, Anindya Mukherjee wrote: > > Thanks for the discussion. This has been very illuminating. I have been > > digging > > around in /usr/src/ and ignoring the atomic architectures (where I got > > stuck) it > > looks like it should be possible to use uint64_t everywhere. I'm playing > > with > > some changes on my machine to see if I can get at least systat(1) and > > vmstat(8) > > to work with uint64_t. The ddb printer (uvmexp_print)is another consumer. > > > > If it works in the base system then ideally every relevant port should be > > updated to be consistent. That is indeed quite a big change; more than I > > realised so thanks for setting me straight on that. > > We have coped with bigger changes in structs like this before, > it didn't used to be too difficult, but that was before go... >
Re: systat(1) counter overflow
On Sat, Jul 03, 2021 at 11:20:42AM +0100, Stuart Henderson wrote:
> On 2021/07/03 01:09, Anindya Mukherjee wrote:
> > Thanks for the discussion. This has been very illuminating. I have been
> > digging
> > around in /usr/src/ and ignoring the atomic architectures (where I got
> > stuck) it
> > looks like it should be possible to use uint64_t everywhere. I'm playing
> > with
> > some changes on my machine to see if I can get at least systat(1) and
> > vmstat(8)
> > to work with uint64_t. The ddb printer (uvmexp_print)is another consumer.
> >
> > If it works in the base system then ideally every relevant port should be
> > updated to be consistent. That is indeed quite a big change; more than I
> > realised so thanks for setting me straight on that.
>
> We have coped with bigger changes in structs like this before,
> it didn't used to be too difficult, but that was before go...
>
Hi,
I have been running for a week with the following diff. This is just a POC and
hence there are a few ugly hacks. So far top(1), systat(1), and vmstat(8) seem
to be happy. I haven't hit the 32-bit overflow point for any counters yet but
the counts look right. I have completely ignored ports, but it looks like the
base system can run with this change. This was mostly to satisfy my curiosity.
Regards,
Anindya
? usr.bin/systat/vim_session
Index: sys/arch/amd64/include/atomic.h
===
RCS file: /cvs/src/sys/arch/amd64/include/atomic.h,v
retrieving revision 1.21
diff -u -p -r1.21 atomic.h
--- sys/arch/amd64/include/atomic.h 11 Mar 2021 11:16:55 - 1.21
+++ sys/arch/amd64/include/atomic.h 13 Jul 2021 07:42:51 -
@@ -150,6 +150,14 @@ _atomic_inc_long(volatile unsigned long
#define atomic_inc_long(_p) _atomic_inc_long(_p)
static inline void
+_atomic_inc_uint64(volatile uint64_t *p)
+{
+ __asm volatile(_LOCK " incq %0"
+ : "+m" (*p));
+}
+#define atomic_inc_uint64(_p) _atomic_inc_uint64(_p)
+
+static inline void
_atomic_dec_int(volatile unsigned int *p)
{
__asm volatile(_LOCK " decl %0"
@@ -166,6 +174,14 @@ _atomic_dec_long(volatile unsigned long
#define atomic_dec_long(_p) _atomic_dec_long(_p)
static inline void
+_atomic_dec_uint64(volatile uint64_t *p)
+{
+ __asm volatile(_LOCK " decq %0"
+ : "+m" (*p));
+}
+#define atomic_dec_uint64(_p) _atomic_dec_uint64(_p)
+
+static inline void
_atomic_add_int(volatile unsigned int *p, unsigned int v)
{
__asm volatile(_LOCK " addl %1,%0"
@@ -182,6 +198,15 @@ _atomic_add_long(volatile unsigned long
: "a" (v));
}
#define atomic_add_long(_p, _v) _atomic_add_long(_p, _v)
+
+static inline void
+_atomic_add_uint64(volatile uint64_t *p, uint64_t v)
+{
+ __asm volatile(_LOCK " addq %1,%0"
+ : "+m" (*p)
+ : "a" (v));
+}
+#define atomic_add_uint64(_p, _v) _atomic_add_uint64(_p, _v)
static inline void
_atomic_sub_int(volatile unsigned int *p, unsigned int v)
Index: sys/sys/sysctl.h
===
RCS file: /cvs/src/sys/sys/sysctl.h,v
retrieving revision 1.218
diff -u -p -r1.218 sysctl.h
--- sys/sys/sysctl.h17 May 2021 17:54:31 - 1.218
+++ sys/sys/sysctl.h13 Jul 2021 07:42:52 -
@@ -38,7 +38,8 @@
#ifndef _SYS_SYSCTL_H_
#define_SYS_SYSCTL_H_
-#include
+/*#include */
+#include "/usr/src/sys/uvm/uvmexp.h"
/*
* Definitions for sysctl call. The sysctl call uses a hierarchical name
Index: sys/uvm/uvm_anon.c
===
RCS file: /cvs/src/sys/uvm/uvm_anon.c,v
retrieving revision 1.54
diff -u -p -r1.54 uvm_anon.c
--- sys/uvm/uvm_anon.c 26 Mar 2021 13:40:05 - 1.54
+++ sys/uvm/uvm_anon.c 13 Jul 2021 07:42:52 -
@@ -119,7 +119,7 @@ uvm_anfree_list(struct vm_anon *anon, st
if (anon->an_swslot != 0) {
/* This page is no longer only in swap. */
KASSERT(uvmexp.swpgonly > 0);
- atomic_dec_int();
+ atomic_dec_uint64();
}
}
anon->an_lock = NULL;
Index: sys/uvm/uvm_aobj.c
===
RCS file: /cvs/src/sys/uvm/uvm_aobj.c,v
retrieving revision 1.99
diff -u -p -r1.99 uvm_aobj.c
--- sys/uvm/uvm_aobj.c 28 Jun 2021 11:19:01 - 1.99
+++ sys/uvm/uvm_aobj.c 13 Jul 2021 07:42:52 -
@@ -1516,6 +1516,6 @@ uao_dropswap_range(struct uvm_object *uo
*/
if (swpgonlydelta > 0) {
KASSERT(uvmexp.swpgonly >= swpgonlydelta);
- atomic_add_int(, -swpgonlydelta);
+ atomic_add_uint64(, -swpgonlydelta);
}
}
Inde
Re: systat(1) counter overflow
One issue I have on amd64 while trying to build the kernel with all uvmexp members promoted to uint64_t is the use of atomic operations. For example, swpgonly is modified atomically in several places and those expect an int. I think only a few members are accessed in this way. On Sat, Jul 03, 2021 at 11:20:42AM +0100, Stuart Henderson wrote: > On 2021/07/03 01:09, Anindya Mukherjee wrote: > > Thanks for the discussion. This has been very illuminating. I have been > > digging > > around in /usr/src/ and ignoring the atomic architectures (where I got > > stuck) it > > looks like it should be possible to use uint64_t everywhere. I'm playing > > with > > some changes on my machine to see if I can get at least systat(1) and > > vmstat(8) > > to work with uint64_t. The ddb printer (uvmexp_print)is another consumer. > > > > If it works in the base system then ideally every relevant port should be > > updated to be consistent. That is indeed quite a big change; more than I > > realised so thanks for setting me straight on that. > > We have coped with bigger changes in structs like this before, > it didn't used to be too difficult, but that was before go... >
Re: systat(1) counter overflow
Thanks for the discussion. This has been very illuminating. I have been digging around in /usr/src/ and ignoring the atomic architectures (where I got stuck) it looks like it should be possible to use uint64_t everywhere. I'm playing with some changes on my machine to see if I can get at least systat(1) and vmstat(8) to work with uint64_t. The ddb printer (uvmexp_print)is another consumer. If it works in the base system then ideally every relevant port should be updated to be consistent. That is indeed quite a big change; more than I realised so thanks for setting me straight on that. Regards, Anindya
systat(1) counter overflow
Hi,
I noticed that if I leave the system running for more than about a month, some
of the counters in the uvm view of systat(1) overflow and become negative. This
is because the members of struct uvmexp in sys/uvm/uvmexp.h are ints. The
kernel's internal counters are of course uint64_t so they don't overflow. It
only happens during the uvm_sysctl(9) call which casts the numbers to integers.
The function is uvmexp_read.
In the attached diff I took the path of least resistance and promoted some of
the counters to unsigned int. Ideally I would have liked to use int64_t or even
uint64_t, but I hit an issue in some of the architecture dependent code. An
example is:
/usr/src/sys/arch/alpha/alpha/trap.c:536 atomic_add_int(, 1);
In other places the ++ operator is used to increment the counters and the 64 bit
types can be used.
I am not completely sure this is the best way to proceed, but even if this diff
is horrifying, I'd appreciate some feedback and advice, thanks!
Anindya
Index: sys/uvm/uvm_meter.c
===
RCS file: /cvs/src/sys/uvm/uvm_meter.c,v
retrieving revision 1.42
diff -u -p -r1.42 uvm_meter.c
--- sys/uvm/uvm_meter.c 28 Dec 2020 14:01:23 - 1.42
+++ sys/uvm/uvm_meter.c 28 Jun 2021 05:24:56 -
@@ -329,7 +329,7 @@ uvmexp_read(struct uvmexp *uexp)
counters_read(uvmexp_counters, counters, exp_ncounters);
/* stat counters */
- uexp->faults = (int)counters[faults];
+ uexp->faults = (unsigned int)counters[faults];
uexp->pageins = (int)counters[pageins];
/* fault subcounters */
@@ -379,10 +379,10 @@ uvmexp_print(int (*pr)(const char *, ...
(*pr)(" freemin=%d, free-target=%d, inactive-target=%d, "
"wired-max=%d\n", uexp.freemin, uexp.freetarg, uexp.inactarg,
uexp.wiredmax);
- (*pr)(" faults=%d, traps=%d, intrs=%d, ctxswitch=%d fpuswitch=%d\n",
+ (*pr)(" faults=%u, traps=%u, intrs=%u, ctxswitch=%u fpuswitch=%d\n",
uexp.faults, uexp.traps, uexp.intrs, uexp.swtch,
uexp.fpswtch);
- (*pr)(" softint=%d, syscalls=%d, kmapent=%d\n",
+ (*pr)(" softint=%u, syscalls=%u, kmapent=%d\n",
uexp.softs, uexp.syscalls, uexp.kmapent);
(*pr)(" fault counts:\n");
Index: sys/uvm/uvmexp.h
===
RCS file: /cvs/src/sys/uvm/uvmexp.h,v
retrieving revision 1.9
diff -u -p -r1.9 uvmexp.h
--- sys/uvm/uvmexp.h4 Mar 2021 09:00:03 - 1.9
+++ sys/uvm/uvmexp.h28 Jun 2021 05:24:56 -
@@ -90,12 +90,12 @@ struct uvmexp {
int unused06; /* formerly nfreeanon */
/* stat counters */
- int faults; /* page fault count */
- int traps; /* trap count */
- int intrs; /* interrupt count */
- int swtch; /* context switch count */
- int softs; /* software interrupt count */
- int syscalls; /* system calls */
+ unsigned int faults;/* page fault count */
+ unsigned int traps; /* trap count */
+ unsigned int intrs; /* interrupt count */
+ unsigned int swtch; /* context switch count */
+ unsigned int softs; /* software interrupt count */
+ unsigned int syscalls; /* system calls */
int pageins;/* pagein operation count */
/* pageouts are in pdpageouts below */
int unused07; /* formerly obsolete_swapins */
Index: usr.bin/systat/uvm.c
===
RCS file: /cvs/src/usr.bin/systat/uvm.c,v
retrieving revision 1.5
diff -u -p -r1.5 uvm.c
--- usr.bin/systat/uvm.c28 Jun 2019 13:35:04 - 1.5
+++ usr.bin/systat/uvm.c28 Jun 2021 05:24:57 -
@@ -37,22 +37,23 @@ void print_uvm(void);
int read_uvm(void);
int select_uvm(void);
-void print_uvmexp_field(field_def *, field_def *, int *, int *, const char *);
+void print_uvmexp_field(field_def *, field_def *, unsigned int *,
+unsigned int *, const char *);
void print_uvmexp_line(int);
struct uvmexp uvmexp;
struct uvmexp last_uvmexp;
struct uvmline {
- int *v1;
- int *ov1;
- char*n1;
- int *v2;
- int *ov2;
- char*n2;
- int *v3;
- int *ov3;
- char*n3;
+ unsigned int*v1;
+ unsigned int*ov1;
+ char*n1;
+ unsigned int*v2;
+ unsigned int*ov2;
+ char*n2;
+ unsigned int*v3;
+ unsigned int*ov3;
+ char*n3;
};
struct uvmline uvmline[] = {
@@ -214,8 +215,8 @@ read_uvm(void)
}
void
-print_uvmexp_field(field_def *fvalue, field_def *fname, int *new, int *old,
-const char *name)
+print_uvmexp_field(field_def *fvalue, field_def *fname,
Re: systat(1) sticky help
I have been using this patch from martijn@ and find it really useful. Is
it possible to merge this? Thanks!
Regards,
Anindya
On Tue, Mar 09, 2021 at 07:33:29PM +0100, Martijn van Duren wrote:
> I send out an earlier version of this diff to Anindya with some positive
> feedback. Instead of claiming another binding, why not make help, order
> and view a toggle? I see no reason why this information should disappear
> on the next input.
>
> Seems to work fine in my testing.
>
> OK?
>
> martijn@
>
> On Thu, 2021-03-04 at 22:47 -0800, Anindya Mukherjee wrote:
> > Hi,
> >
> > I have reworked my proposed interface for sticky displays in systat
> > following earlier feedback. Thanks for that! It helped me rethink the
> > interface carefully.
> >
> > In the current version, sticky mode has its own toggle: ^T. From a study
> > of the source and the man page it does not seem to be bound to anything
> > and, on my system at least, does not conflict with anything else. I
> > could add a dedicated command for it as well if that's useful. The
> > current version supports a sticky display for the view list, view mode
> > plus refresh interval, and available orderings.
> >
> > I have tried the hotkeys for the various views as documented in the man
> > page (as well as some of the undocumented ones) and they seem to work. I
> > have also tried to make sure that the sticky displays don't interfere
> > with error messages. In future I would like to try to clean up the view
> > hotkeys, which conflict each other in some cases, and seem to be
> > undocumented. I found them useful; at least the ones that work. Please
> > have a look, thanks!
> >
> > Regards,
> > Anindya
>
> Index: engine.c
> ===
> RCS file: /cvs/src/usr.bin/systat/engine.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 engine.c
> --- engine.c 6 Feb 2021 06:19:28 - 1.27
> +++ engine.c 9 Mar 2021 18:32:24 -
> @@ -91,6 +91,8 @@ char cmdbuf[MAX_LINE_BUF];
> int cmd_len = -1;
> struct command *curr_cmd = NULL;
> char *curr_message = NULL;
> +enum message_mode message_mode = MESSAGE_NONE;
> +int message_cont = 1;
>
> void print_cmdline(void);
>
> @@ -1145,14 +1147,26 @@ command_set(struct command *cmd, const c
> return prev;
> }
>
> +void
> +message_toggle(enum message_mode mode)
> +{
> + message_mode = message_mode != mode ? mode : MESSAGE_NONE;
> + need_update = 1;
> + message_cont = 1;
> +}
> +
> const char *
> -message_set(const char *msg) {
> - char *prev = curr_message;
> - if (msg)
> +message_set(const char *msg)
> +{
> + free(curr_message);
> +
> + if (msg) {
> curr_message = strdup(msg);
> - else
> + message_cont = 0;
> + } else {
> curr_message = NULL;
> - free(prev);
> + message_cont = 1;
> + }
> return NULL;
> }
>
> @@ -1361,6 +1375,22 @@ engine_loop(int countmax)
> if (!averageonly ||
> (averageonly && count == countmax - 1))
> disp_update();
> + if (message_cont) {
> + switch (message_mode) {
> + case MESSAGE_NONE:
> + message_set(NULL);
> + break;
> + case MESSAGE_HELP:
> + show_help();
> + break;
> + case MESSAGE_VIEW:
> + show_view();
> + break;
> + case MESSAGE_ORDER:
> + show_order();
> + break;
> + }
> + }
> end_page();
> need_update = 0;
> if (countmax && ++count >= countmax)
> Index: engine.h
> ===
> RCS file: /cvs/src/usr.bin/systat/engine.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 engine.h
> --- engine.h 12 Jan 2020 20:51:08 - 1.12
> +++ engine.h 9 Mar 2021 18:32:24 -
> @@ -95,6 +95,12 @@ struct command {
> void ( *exec)(const char *);
> };
>
> +enum message_mode {
> + MESSAGE_NONE,
> + MESSAGE_HELP,
> + MESSAGE_VIEW,
> + MESSAGE_ORDER
> +};
Re: patch: make ld.so aware of pthread_key_create destructor - Re: multimedia/mpv debug vo=gpu crash on exit
On Sat, May 08, 2021 at 07:26:32AM +0200, Sebastien Marie wrote: > On Thu, May 06, 2021 at 06:23:08PM -0700, Anindya Mukherjee wrote: > > On Thu, May 06, 2021 at 08:00:56AM -0600, Todd C. Miller wrote: > > > On Thu, 06 May 2021 09:32:28 +0200, Sebastien Marie wrote: > > > > > > > We already take care of such situation with __cxa_thread_atexit_impl > > > > (in libc/stdlib/thread_atexit.c), by keeping an additionnal reference > > > > on object loaded (it makes ld.so aware that it is still used and so > > > > dlclose() doesn't unload it). > > > > > > > > I used the same idiom for pthread_key_create() and used dlctl(3) in > > > > the same way with the destructor address. > > > > > > This will set STAT_NODELETE so the DSO will never really get unloaded. > > > That's not a problem for atexit() since the process is headed for > > > the exit. > > > > > > I'm less sure about using it here since we don't have a way to > > > unreference the DSO upon pthread_key_delete(). > > > > > > - todd > > > > I did a quick investigation on my Linux machine and there mpv seems to > > be using libEGL_mesa.so instead of iris_dri.so. In this case I am not > > seeing a call to pthread_key_create at the start of video playback > > (there are some other places where pthread_key_create is called from but > > they don't cause a problem). So, not sure what happens in Linux when > > iris_dri.so is used. > > libEGL_mesa.so seems to be used when mesa is built with 'with_glvnd' > option. glvnd is "vendor-neutral libGL" : > https://gitlab.freedesktop.org/glvnd/libglvnd > This is very interesting! Since Arch Linux's version of Mesa is indeed build with glvnd enabled: https://github.com/archlinux/svntogit-packages/blob/packages/mesa/trunk/PKGBUILD#L53 it avoids this issue. So, it seems the main problem is in iris_dri.so. > > > However, the Linux implementation of > > pthread_key_create seems to also not increment the refcount when the > > destructor is set so I don't yet see how it's solved there, assuming > > iris_dri.so behaves identically. > > glibc seems to have the same problem with pthread_key_create(): > https://sourceware.org/bugzilla/show_bug.cgi?id=21032 > and the bugreport reference a simple poc at > https://github.com/Aaron1011/pthread_dlopen > Thanks! I found some of these as well while searching for related issues on Linux. So it seems this is not OpenBSD specific, as I suspected. > > -- > Sebastien Marie Regards, Anindya
Re: patch: make ld.so aware of pthread_key_create destructor - Re: multimedia/mpv debug vo=gpu crash on exit
On Thu, May 06, 2021 at 08:00:56AM -0600, Todd C. Miller wrote: > On Thu, 06 May 2021 09:32:28 +0200, Sebastien Marie wrote: > > > We already take care of such situation with __cxa_thread_atexit_impl > > (in libc/stdlib/thread_atexit.c), by keeping an additionnal reference > > on object loaded (it makes ld.so aware that it is still used and so > > dlclose() doesn't unload it). > > > > I used the same idiom for pthread_key_create() and used dlctl(3) in > > the same way with the destructor address. > > This will set STAT_NODELETE so the DSO will never really get unloaded. > That's not a problem for atexit() since the process is headed for > the exit. > > I'm less sure about using it here since we don't have a way to > unreference the DSO upon pthread_key_delete(). For sure I don't fully appreciate the complexities involved here, but is it possible to store the shared object handle along with the destructor when the reference count is incremented in the above patch? Then we could use that to decrement the reference. > > - todd Thanks for looking into this. Regards, Anindya
Re: patch: make ld.so aware of pthread_key_create destructor - Re: multimedia/mpv debug vo=gpu crash on exit
On Thu, May 06, 2021 at 08:00:56AM -0600, Todd C. Miller wrote: > On Thu, 06 May 2021 09:32:28 +0200, Sebastien Marie wrote: > > > We already take care of such situation with __cxa_thread_atexit_impl > > (in libc/stdlib/thread_atexit.c), by keeping an additionnal reference > > on object loaded (it makes ld.so aware that it is still used and so > > dlclose() doesn't unload it). > > > > I used the same idiom for pthread_key_create() and used dlctl(3) in > > the same way with the destructor address. > > This will set STAT_NODELETE so the DSO will never really get unloaded. > That's not a problem for atexit() since the process is headed for > the exit. > > I'm less sure about using it here since we don't have a way to > unreference the DSO upon pthread_key_delete(). > > - todd I did a quick investigation on my Linux machine and there mpv seems to be using libEGL_mesa.so instead of iris_dri.so. In this case I am not seeing a call to pthread_key_create at the start of video playback (there are some other places where pthread_key_create is called from but they don't cause a problem). So, not sure what happens in Linux when iris_dri.so is used. However, the Linux implementation of pthread_key_create seems to also not increment the refcount when the destructor is set so I don't yet see how it's solved there, assuming iris_dri.so behaves identically. Regards, Anindya
Re: systat(1) sticky help
TICKY_HELP, STICKY_VIEW, STICKY_ORDER};
void tb_start(void);
-
void tb_end(void);
int tbprintf(char *format, ...) GCC_PRINTFLIKE(1,2);
@@ -144,6 +145,9 @@ struct command *command_set(struct comma
const char *message_set(const char *msg);
void foreach_view(void (*callback)(field_view *));
+void show_help(void);
+void show_view(void);
+void show_order(void);
extern int sortdir;
extern useconds_t udelay;
@@ -160,6 +164,9 @@ extern int columns, lines;
extern int need_update;
extern int need_sort;
extern int separate_thousands;
+extern enum sticky_mode sticky_mode;
+extern int sticky;
+extern char *curr_message;
extern volatile sig_atomic_t gotsig_close;
extern volatile sig_atomic_t gotsig_resize;
Index: main.c
===
RCS file: /cvs/src/usr.bin/systat/main.c,v
retrieving revision 1.73
diff -u -p -r1.73 main.c
--- main.c 30 Jan 2021 08:44:42 - 1.73
+++ main.c 5 Mar 2021 02:35:36 -
@@ -150,6 +150,7 @@ error(const char *fmt, ...)
va_end(ap);
message_set(buf);
+ sticky_mode = STICKY_NONE;
}
void
@@ -285,6 +286,7 @@ cmd_compat(const char *buf)
if (strcasecmp(buf, "help") == 0) {
show_help();
+ sticky_mode = STICKY_HELP;
need_update = 1;
return;
}
@@ -305,6 +307,7 @@ cmd_compat(const char *buf)
}
if (strncasecmp(buf, "order", 5) == 0) {
show_order();
+ sticky_mode = STICKY_ORDER;
need_update = 1;
return;
}
@@ -359,11 +362,27 @@ keyboard_callback(int ch)
/* FALLTHROUGH */
case 'h':
show_help();
+ sticky_mode = STICKY_HELP;
need_update = 1;
break;
case CTRL_G:
show_view();
+ sticky_mode = STICKY_VIEW;
need_update = 1;
+ break;
+ case CTRL_T:
+ if (sticky) {
+ sticky = 0;
+ sticky_mode = STICKY_NONE;
+ message_set("sticky off");
+ need_update = 1;
+ } else {
+ sticky = 1;
+ if (curr_message == NULL ||
+ strcmp(curr_message, "sticky off") == 0)
+ message_set("sticky on");
+ need_update = 1;
+ }
break;
case 'l':
command_set(_count, NULL);
Index: systat.1
===
RCS file: /cvs/src/usr.bin/systat/systat.1,v
retrieving revision 1.119
diff -u -p -r1.119 systat.1
--- systat.122 Jun 2020 13:17:54 - 1.119
+++ systat.15 Mar 2021 02:35:36 -
@@ -175,6 +175,8 @@ line typed as a command.
While entering a command the
current character erase, word erase, and line kill characters
may be used.
+.It Ic h
+Print the names of the available views on the command line.
.It Ic o
Select the next ordering which sorts the rows according to a
combination of columns.
@@ -209,6 +211,11 @@ Refresh the screen.
Scroll current view down by one line.
.It Ic ^P | Aq Ic up arrow
Scroll current view up by one line.
+.It Ic ^T
+Toggle sticky mode. When active, the status line display is
+not cleared on a key press while showing the available views,
+the current view, or the available orderings. The status
+line displays an asterisk in sticky mode when it is not empty.
.It Ic ^V | Aq Ic Page Down
Scroll current view down by one page.
.It Ic Alt-V | Aq Ic Page Up
On Mon, Mar 01, 2021 at 08:37:58AM +0100, Martijn van Duren wrote:
> On Sun, 2021-02-28 at 23:03 -0800, Anindya Mukherjee wrote:
> > Hi,
> >
> > Thanks for the feedback. I see your point. Perhaps it would be better to
> > have a separate "stickiness" toggle and not tie it to the help or the
> > view name display functions. For example, pressing 't' could toggle it
> > on and then pressing 'h' or Ctrl-G would make the display stick.
> > Pressing 't' again would toggle it off, and then the display would get
> > cleared normally.
>
> Something like that seems better, but keep in mind that the single key
> bindings within systat is a mess: 't' might already be allocated by one
> of the views. When submitting your followup, try to make sure that you
> considered all possible issues, including the keybindings, and show
> how you came to your conclusion.
> >
> > Regards,
> > Anindya
> >
> > On Mon, Mar 01, 2021 at 07:49:49AM +0100, Martijn van Duren wrote:
> > > Although the feature *might* be useful (just woken up, so haven't given
> > > it too much thought), the bindings have some problems and need
Re: systat(1) sticky help
Hi,
Thanks for the feedback. I see your point. Perhaps it would be better to
have a separate "stickiness" toggle and not tie it to the help or the
view name display functions. For example, pressing 't' could toggle it
on and then pressing 'h' or Ctrl-G would make the display stick.
Pressing 't' again would toggle it off, and then the display would get
cleared normally.
Regards,
Anindya
On Mon, Mar 01, 2021 at 07:49:49AM +0100, Martijn van Duren wrote:
> Although the feature *might* be useful (just woken up, so haven't given
> it too much thought), the bindings have some problems and need to be
> reworked if we decide to get this in. Problems that I see from just a
> few short tests (so there might be others):
> - Entering sticky: H, which also enters help, which is annoying if I
> want sticky ^G.
> - Clearing sticky: h, which also displays help, which if I come from
> ^G is not what I want and if I'm in help is confusing, because I want
> to get rid of it.
>
> martijn@
>
> On Sun, 2021-02-28 at 19:40 -0800, Anindya Mukherjee wrote:
> > Hi,
> >
> > I tend to keep systat(1) running in interactive mode, and find it very
> > useful to have the help line displayed permanently, i.e., not disappear
> > if a key is pressed or after a command is executed. The same goes for
> > the Ctrl-G display. For example, it tells me what modes are adjacent to
> > the currently active one when pressing left or right arrow keys.
> >
> > The attached diff adds this feature. It is off by default. I noticed
> > that the 'h' key was not documented in the man page, so I took the
> > liberty of adding a line describing it.
> >
> > Hope it is useful. Please have a look, thanks!
> >
> > Regards,
> > Anindya
> >
> > Index: engine.c
> > ===
> > RCS file: /cvs/src/usr.bin/systat/engine.c,v
> > retrieving revision 1.27
> > diff -u -p -r1.27 engine.c
> > --- engine.c6 Feb 2021 06:19:28 - 1.27
> > +++ engine.c1 Mar 2021 03:20:41 -
> > @@ -70,6 +70,8 @@ volatile sig_atomic_t gotsig_alarm = 0;
> > int need_update = 0;
> > int need_sort = 0;
> > int separate_thousands = 0;
> > +int viewmode = 0;
> > +int sticky = 0;
> >
> > SCREEN *screen;
> >
> > @@ -1139,7 +1141,9 @@ command_set(struct command *cmd, const c
> > cmdbuf[0] = 0;
> > }
> > }
> > - message_set(NULL);
> > +
> > + if (!sticky)
> > + message_set(NULL);
> > curr_cmd = cmd;
> > need_update = 1;
> > return prev;
> > @@ -1234,7 +1238,7 @@ keyboard(void)
> > return;
> >
> > if (curr_message != NULL) {
> > - if (ch > 0) {
> > + if (ch > 0 && !sticky) {
> > message_set(NULL);
> > need_update = 1;
> > }
> > @@ -1359,8 +1363,15 @@ engine_loop(int countmax)
> > if (need_update) {
> > erase();
> > if (!averageonly ||
> > - (averageonly && count == countmax - 1))
> > + (averageonly && count == countmax - 1)) {
> > disp_update();
> > + if (interactive && sticky) {
> > + if (viewmode)
> > + show_view();
> > + else
> > + show_help();
> > + }
> > + }
> > end_page();
> > need_update = 0;
> > if (countmax && ++count >= countmax)
> > Index: engine.h
> > ===
> > RCS file: /cvs/src/usr.bin/systat/engine.h,v
> > retrieving revision 1.12
> > diff -u -p -r1.12 engine.h
> > --- engine.h12 Jan 2020 20:51:08 - 1.12
> > +++ engine.h1 Mar 2021 03:20:41 -
> > @@ -144,6 +144,8 @@ struct command *command_set(struct comma
> > const char *message_set(const char *msg);
> >
> > void foreach_view(void (*callback)(field_view *));
> > +void show_help(void);
> > +void show_view(void);
> >
> > extern int sortdir;
> > exter
systat(1) sticky help
Hi,
I tend to keep systat(1) running in interactive mode, and find it very
useful to have the help line displayed permanently, i.e., not disappear
if a key is pressed or after a command is executed. The same goes for
the Ctrl-G display. For example, it tells me what modes are adjacent to
the currently active one when pressing left or right arrow keys.
The attached diff adds this feature. It is off by default. I noticed
that the 'h' key was not documented in the man page, so I took the
liberty of adding a line describing it.
Hope it is useful. Please have a look, thanks!
Regards,
Anindya
Index: engine.c
===
RCS file: /cvs/src/usr.bin/systat/engine.c,v
retrieving revision 1.27
diff -u -p -r1.27 engine.c
--- engine.c6 Feb 2021 06:19:28 - 1.27
+++ engine.c1 Mar 2021 03:20:41 -
@@ -70,6 +70,8 @@ volatile sig_atomic_t gotsig_alarm = 0;
int need_update = 0;
int need_sort = 0;
int separate_thousands = 0;
+int viewmode = 0;
+int sticky = 0;
SCREEN *screen;
@@ -1139,7 +1141,9 @@ command_set(struct command *cmd, const c
cmdbuf[0] = 0;
}
}
- message_set(NULL);
+
+ if (!sticky)
+ message_set(NULL);
curr_cmd = cmd;
need_update = 1;
return prev;
@@ -1234,7 +1238,7 @@ keyboard(void)
return;
if (curr_message != NULL) {
- if (ch > 0) {
+ if (ch > 0 && !sticky) {
message_set(NULL);
need_update = 1;
}
@@ -1359,8 +1363,15 @@ engine_loop(int countmax)
if (need_update) {
erase();
if (!averageonly ||
- (averageonly && count == countmax - 1))
+ (averageonly && count == countmax - 1)) {
disp_update();
+ if (interactive && sticky) {
+ if (viewmode)
+ show_view();
+ else
+ show_help();
+ }
+ }
end_page();
need_update = 0;
if (countmax && ++count >= countmax)
Index: engine.h
===
RCS file: /cvs/src/usr.bin/systat/engine.h,v
retrieving revision 1.12
diff -u -p -r1.12 engine.h
--- engine.h12 Jan 2020 20:51:08 - 1.12
+++ engine.h1 Mar 2021 03:20:41 -
@@ -144,6 +144,8 @@ struct command *command_set(struct comma
const char *message_set(const char *msg);
void foreach_view(void (*callback)(field_view *));
+void show_help(void);
+void show_view(void);
extern int sortdir;
extern useconds_t udelay;
@@ -160,6 +162,8 @@ extern int columns, lines;
extern int need_update;
extern int need_sort;
extern int separate_thousands;
+extern int viewmode;
+extern int sticky;
extern volatile sig_atomic_t gotsig_close;
extern volatile sig_atomic_t gotsig_resize;
Index: main.c
===
RCS file: /cvs/src/usr.bin/systat/main.c,v
retrieving revision 1.73
diff -u -p -r1.73 main.c
--- main.c 30 Jan 2021 08:44:42 - 1.73
+++ main.c 1 Mar 2021 03:20:41 -
@@ -285,6 +285,14 @@ cmd_compat(const char *buf)
if (strcasecmp(buf, "help") == 0) {
show_help();
+ sticky = 0;
+ need_update = 1;
+ return;
+ }
+ if (strcasecmp(buf, "stickyhelp") == 0) {
+ show_help();
+ sticky = 1;
+ viewmode = 0;
need_update = 1;
return;
}
@@ -359,10 +367,18 @@ keyboard_callback(int ch)
/* FALLTHROUGH */
case 'h':
show_help();
+ sticky = 0;
+ need_update = 1;
+ break;
+ case 'H':
+ show_help();
+ sticky = 1;
+ viewmode = 0;
need_update = 1;
break;
case CTRL_G:
show_view();
+ viewmode = 1;
need_update = 1;
break;
case 'l':
Index: systat.1
===
RCS file: /cvs/src/usr.bin/systat/systat.1,v
retrieving revision 1.119
diff -u -p -r1.119 systat.1
--- systat.122 Jun 2020 13:17:54 - 1.119
+++ systat.11 Mar 2021 03:20:41 -
@@ -175,6 +175,8 @@ line typed as a command.
While entering a command the
current character erase, word erase, and line kill characters
may be used.
+.It Ic h
+Print the names of the available views on the command line.
.It Ic o
Select the
Re: Workflow question
Thanks! This works perfectly! On Sun, Feb 28, 2021 at 12:01:06AM +0100, Theo Buehler wrote: > > Following the advice in the FAQ I added my user to the wobj group. I > > suppose I could "make obj" and have the objs written to /usr/obj? Is > > this a workflow the developers recommend or follow? Thanks! > > Yes. More precisely, by default 'make obj' in /usr/src/usr.bin/systat > will create a symlink obj@ -> /usr/obj/usr.bin/systat where the build > artefacts will be placed. > > $ ls -ld /usr/obj/usr.bin/systat > drwxrwx--- 2 build wobj 1024 Feb 26 21:47 /usr/obj/usr.bin/systat/ Regards, Anindya
Workflow question
Hi, I am making a small change to systat(1) and hope to submit it for review soon. For my own edification, I have a few questions regarding the recommended workflow for such work. Hopefully tech@ is the right place to ask this :) I generated the tags with "make tags", and also temporarily added -O0 -g to the build flags in the Makefile. I can run "make" in /usr/src/usr.bin/systat directly and it works: I get a binary which runs. However, I don't like the fact that it writes the obj files in the same directory. Following the advice in the FAQ I added my user to the wobj group. I suppose I could "make obj" and have the objs written to /usr/obj? Is this a workflow the developers recommend or follow? Thanks! Anindya
Re: Mesa leak in intel iris driver
Hi, I have been noticing this leak for a few weeks now. I typically update to the latest snapshot every 10-14 days and Xorg memory usage grows monotonically in the meantime. I was gathering some data for a report, but thanks to this thread it's explained now. I look forward to testing the fix. I also have the iris driver. > 35MB of memory is too large to expose amap leakage, since most of the amaps > are associated with single pages. The X server does some very large > allocations, > and thus SIZE will not show these very well. > > To see the problem, It is better to look at "UVM amap" in "vmstat -m" > > UVM amap 32835 1690K 2636K 78644K 268129080 > 16,32,64,128,256,512,1024,4096,8192 > > this number is way too big, it should be 500 to 2000 ish. Thanks for this comment! Now I know what to look for. On my system running for 3 days it is already over 32,000. Regards, Anindya
Re: uhidpp(4): logitech hid++ device driver
[b5 03 00] hidpp_send_report: 10 ff 83 b5 [35 00 00] uhidpp_intr: 10 ff 8f 83 [b5 03 00] hidpp_send_report: 10 ff 80 00 [10 09 00] uhidpp_intr: 10 ff 80 00 [00 00 00] uhid6 at uhidev4 reportid 32: input=14, output=14, feature=0 uhid7 at uhidev4 reportid 33: input=31, output=31, feature=0 uaudio0 at uhub0 port 10 configuration 1 interface 1 "C-Media Electronics Inc. Microsoft LifeChat LX-3000" rev 1.10/1.00 addr 4 uaudio0: class v1, full-speed, sync, channels: 2 play, 1 rec, 8 ctls audio1 at uaudio0 uhidev5 at uhub0 port 10 configuration 1 interface 3 "C-Media Electronics Inc. Microsoft LifeChat LX-3000" rev 1.10/1.00 addr 4 uhidev5: iclass 3/0 uhid8 at uhidev5: input=4, output=4, feature=0 vscsi0 at root scsibus2 at vscsi0: 256 targets softraid0 at root scsibus3 at softraid0: 256 targets root on sd0a (72b083e204d4e2ab.a) swap on sd0b dump on sd0b inteldrm0: 1920x1080, 32bpp wsdisplay0 at inteldrm0 mux 1: console (std, vt100 emulation), using wskbd0 wskbd1: connecting to wsdisplay0 wskbd2: connecting to wsdisplay0 wskbd3: connecting to wsdisplay0 wskbd4: connecting to wsdisplay0 wskbd5: connecting to wsdisplay0 wsdisplay0: screen 1-5 added (std, vt100 emulation) On Sun, Feb 14, 2021 at 03:48:16PM +0100, Anton Lindqvist wrote: > On Fri, Feb 12, 2021 at 03:48:51AM -0800, Anindya Mukherjee wrote: > > Hi, > > > > Sorry for the delay. I am running the latest snapshot: > > kern.version=OpenBSD 6.9-beta (GENERIC.MP) #331: Thu Feb 11 20:28:45 MST > > 2021 > > dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP > > which seems to have the latest updates. However I still do not see battery > > levels for my M570. Full dmesg below. Please let me know if I can test > > anything. > > Could you try a kernel with revision 1.10 of dev/usb/uhidpp.c present > and UHIDPP_DEBUG enabled. I have also prepared an amd64 kernel with the > necessary bits available here: > > https://www.basename.se/tmp/bsd.uhidpp > > Please send me the complete dmesg from this kernel. It won't make any > battery sensors appear but rather help diagnose.
Re: uhidpp(4): logitech hid++ device driver
(std, vt100 emulation), using wskbd0
wskbd1: connecting to wsdisplay0
wskbd2: connecting to wsdisplay0
wskbd3: connecting to wsdisplay0
wskbd4: connecting to wsdisplay0
wskbd5: connecting to wsdisplay0
wsdisplay0: screen 1-5 added (std, vt100 emulation)
On Tue, Feb 09, 2021 at 08:34:00AM +0100, Anton Lindqvist wrote:
> Hi,
>
> On Mon, Feb 08, 2021 at 02:50:39PM -0800, Anindya Mukherjee wrote:
> > Hi, I have a Logitech M570 which seems to be handled by this new driver.
> > However I don't see any battery level information.
> >
> > dmesg:
> > uhidpp0 at uhidev4 device 1 mouse "M570" serial ef-81-ff-80
> >
> > sysctl kern.version:
> > kern.version=OpenBSD 6.8-current (GENERIC.MP) #313: Fri Feb 5 18:31:44 MST
> > 2021
> > dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> >
> > hw.sensors.uhidpp0 does not seem to exist.
> >
> > Regards,
> > Anindya
> >
>
> Could you try the following diff and send me the complete dmesg. I've
> also prepared an amd64 kernel with the patch applied:
>
> https://www.basename.se/tmp/bsd.uhidpp.battery
>
> diff --git sys/dev/usb/uhidpp.c sys/dev/usb/uhidpp.c
> index b041d86fecd..27f6137ec06 100644
> --- sys/dev/usb/uhidpp.c
> +++ sys/dev/usb/uhidpp.c
> @@ -29,7 +29,7 @@
> #include
> #include
>
> -/* #define UHIDPP_DEBUG */
> +#define UHIDPP_DEBUG
> #ifdef UHIDPP_DEBUG
>
> #define DPRINTF(x...) do { \
> @@ -84,12 +84,16 @@ int uhidpp_debug = 1;
> #define HIDPP_GET_LONG_REGISTER 0x83
>
> #define HIDPP_REG_ENABLE_REPORTS 0x00
> +#define HIDPP_REG_BATTERY_STATUS 0x07
> #define HIDPP_REG_PAIRING_INFORMATION0xb5
>
> #define HIDPP_NOTIF_DEVICE_BATTERY_STATUS(1 << 4)
> #define HIDPP_NOTIF_RECEIVER_WIRELESS(1 << 0)
> #define HIDPP_NOTIF_RECEIVER_SOFTWARE_PRESENT(1 << 3)
>
> +/* Number of battery levels supported by HID++ 1.0 devices. */
> +#define HIDPP_BATTERY_NLEVELS7
> +
> /* HID++ 1.0 error codes. */
> #define HIDPP_ERROR 0x8f
> #define HIDPP_ERROR_SUCCESS 0x00
> @@ -112,7 +116,6 @@ int uhidpp_debug = 1;
> * greater than zero which is reserved for notifications.
> */
> #define HIDPP_SOFTWARE_ID0x01
> -#define HIDPP_SOFTWARE_ID_MASK 0x0f
> #define HIDPP_SOFTWARE_ID_LEN4
>
> #define HIDPP20_FEAT_ROOT_IDX0x00
> @@ -154,8 +157,8 @@ int uhidpp_debug = 1;
>
> /* Feature access report used by the HID++ 2.0 (and greater) protocol. */
> struct fap {
> - uint8_t feature_index;
> - uint8_t funcindex_clientid;
> + uint8_t feature_idx;
> + uint8_t funcidx_swid;
> uint8_t params[HIDPP_REPORT_LONG_PARAMS_MAX];
> };
>
> @@ -185,6 +188,8 @@ struct uhidpp_notification {
> struct uhidpp_device {
> uint8_t d_id;
> uint8_t d_connected;
> + uint8_t d_major;
> + uint8_t d_minor;
> struct {
> struct ksensor b_sens[UHIDPP_NSENSORS];
> uint8_t b_feature_idx;
> @@ -237,8 +242,10 @@ struct uhidpp_notification
> *uhidpp_claim_notification(struct uhidpp_softc *);
> int uhidpp_consume_notification(struct uhidpp_softc *, struct uhidpp_report
> *);
> int uhidpp_is_notification(struct uhidpp_softc *, struct uhidpp_report *);
>
> -int hidpp_get_protocol_version(struct uhidpp_softc *, uint8_t, int *, int
> *);
> +int hidpp_get_protocol_version(struct uhidpp_softc *, uint8_t, uint8_t *,
> +uint8_t *);
>
> +int hidpp10_get_battery_status(struct uhidpp_softc *, uint8_t, uint8_t *);
> int hidpp10_get_name(struct uhidpp_softc *, uint8_t, char *, size_t);
> int hidpp10_get_serial(struct uhidpp_softc *, uint8_t, uint8_t *, size_t);
> int hidpp10_get_type(struct uhidpp_softc *, uint8_t, const char **);
> @@ -520,7 +527,7 @@ void
> uhidpp_device_connect(struct uhidpp_softc *sc, struct uhidpp_device *dev)
> {
> struct ksensor *sens;
> - int error, major, minor;
> + int error;
> uint8_t feature_type;
>
> MUTEX_ASSERT_LOCKED(>sc_mtx);
> @@ -529,30 +536,40 @@ uhidpp_device_connect(struct uhidpp_softc *sc, struct
> uhidpp_device *dev)
> if (dev->d_connected)
> return;
>
> - error = hidpp_get_protocol_version(sc, dev->d_id, , );
> + error = hidpp_get_protocol_version(sc, dev->d_id,
> + >d_major, >d_minor);
> if (error) {
> - DPRINTF("%
Re: uhidpp(4): logitech hid++ device driver
On Tue, Feb 09, 2021 at 08:34:00AM +0100, Anton Lindqvist wrote:
> Hi,
>
> On Mon, Feb 08, 2021 at 02:50:39PM -0800, Anindya Mukherjee wrote:
> > Hi, I have a Logitech M570 which seems to be handled by this new driver.
> > However I don't see any battery level information.
> >
> > dmesg:
> > uhidpp0 at uhidev4 device 1 mouse "M570" serial ef-81-ff-80
> >
> > sysctl kern.version:
> > kern.version=OpenBSD 6.8-current (GENERIC.MP) #313: Fri Feb 5 18:31:44 MST
> > 2021
> > dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> >
> > hw.sensors.uhidpp0 does not seem to exist.
> >
> > Regards,
> > Anindya
> >
>
> Could you try the following diff and send me the complete dmesg. I've
> also prepared an amd64 kernel with the patch applied:
>
> https://www.basename.se/tmp/bsd.uhidpp.battery
Thanks! I'll try it as soon as I can and report back.
>
> diff --git sys/dev/usb/uhidpp.c sys/dev/usb/uhidpp.c
> index b041d86fecd..27f6137ec06 100644
> --- sys/dev/usb/uhidpp.c
> +++ sys/dev/usb/uhidpp.c
> @@ -29,7 +29,7 @@
> #include
> #include
>
> -/* #define UHIDPP_DEBUG */
> +#define UHIDPP_DEBUG
> #ifdef UHIDPP_DEBUG
>
> #define DPRINTF(x...) do { \
> @@ -84,12 +84,16 @@ int uhidpp_debug = 1;
> #define HIDPP_GET_LONG_REGISTER 0x83
>
> #define HIDPP_REG_ENABLE_REPORTS 0x00
> +#define HIDPP_REG_BATTERY_STATUS 0x07
> #define HIDPP_REG_PAIRING_INFORMATION0xb5
>
> #define HIDPP_NOTIF_DEVICE_BATTERY_STATUS(1 << 4)
> #define HIDPP_NOTIF_RECEIVER_WIRELESS(1 << 0)
> #define HIDPP_NOTIF_RECEIVER_SOFTWARE_PRESENT(1 << 3)
>
> +/* Number of battery levels supported by HID++ 1.0 devices. */
> +#define HIDPP_BATTERY_NLEVELS7
> +
> /* HID++ 1.0 error codes. */
> #define HIDPP_ERROR 0x8f
> #define HIDPP_ERROR_SUCCESS 0x00
> @@ -112,7 +116,6 @@ int uhidpp_debug = 1;
> * greater than zero which is reserved for notifications.
> */
> #define HIDPP_SOFTWARE_ID0x01
> -#define HIDPP_SOFTWARE_ID_MASK 0x0f
> #define HIDPP_SOFTWARE_ID_LEN4
>
> #define HIDPP20_FEAT_ROOT_IDX0x00
> @@ -154,8 +157,8 @@ int uhidpp_debug = 1;
>
> /* Feature access report used by the HID++ 2.0 (and greater) protocol. */
> struct fap {
> - uint8_t feature_index;
> - uint8_t funcindex_clientid;
> + uint8_t feature_idx;
> + uint8_t funcidx_swid;
> uint8_t params[HIDPP_REPORT_LONG_PARAMS_MAX];
> };
>
> @@ -185,6 +188,8 @@ struct uhidpp_notification {
> struct uhidpp_device {
> uint8_t d_id;
> uint8_t d_connected;
> + uint8_t d_major;
> + uint8_t d_minor;
> struct {
> struct ksensor b_sens[UHIDPP_NSENSORS];
> uint8_t b_feature_idx;
> @@ -237,8 +242,10 @@ struct uhidpp_notification
> *uhidpp_claim_notification(struct uhidpp_softc *);
> int uhidpp_consume_notification(struct uhidpp_softc *, struct uhidpp_report
> *);
> int uhidpp_is_notification(struct uhidpp_softc *, struct uhidpp_report *);
>
> -int hidpp_get_protocol_version(struct uhidpp_softc *, uint8_t, int *, int
> *);
> +int hidpp_get_protocol_version(struct uhidpp_softc *, uint8_t, uint8_t *,
> +uint8_t *);
>
> +int hidpp10_get_battery_status(struct uhidpp_softc *, uint8_t, uint8_t *);
> int hidpp10_get_name(struct uhidpp_softc *, uint8_t, char *, size_t);
> int hidpp10_get_serial(struct uhidpp_softc *, uint8_t, uint8_t *, size_t);
> int hidpp10_get_type(struct uhidpp_softc *, uint8_t, const char **);
> @@ -520,7 +527,7 @@ void
> uhidpp_device_connect(struct uhidpp_softc *sc, struct uhidpp_device *dev)
> {
> struct ksensor *sens;
> - int error, major, minor;
> + int error;
> uint8_t feature_type;
>
> MUTEX_ASSERT_LOCKED(>sc_mtx);
> @@ -529,30 +536,40 @@ uhidpp_device_connect(struct uhidpp_softc *sc, struct
> uhidpp_device *dev)
> if (dev->d_connected)
> return;
>
> - error = hidpp_get_protocol_version(sc, dev->d_id, , );
> + error = hidpp_get_protocol_version(sc, dev->d_id,
> + >d_major, >d_minor);
> if (error) {
> - DPRINTF("%s: protocol version failure: device_id=%d,
> error=%d\n",
> + DPRINTF("%s: protocol version failure: device_id=%d, "
> + "error=%d\n",
>
Re: uhidpp(4): logitech hid++ device driver
Hi, I have a Logitech M570 which seems to be handled by this new driver. However I don't see any battery level information. dmesg: uhidpp0 at uhidev4 device 1 mouse "M570" serial ef-81-ff-80 sysctl kern.version: kern.version=OpenBSD 6.8-current (GENERIC.MP) #313: Fri Feb 5 18:31:44 MST 2021 dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP hw.sensors.uhidpp0 does not seem to exist. Regards, Anindya
