Re: 7.3: Squid 6.3 with multiple workers - net.unix.dgram.sendspace too low

[Silamael Darkomen]

On 20 Oct 2023 19:33, Stuart Henderson wrote:

After a few hours digging around, I eventually figured out where the
relevant sockets are created and have added a patch (to 7.4-stable and
-current) to bump buffers on them.


Hi Stuart,

Meanwhile I also did some digging and opened a bug at Squid with a patch 
attached: https://bugs.squid-cache.org/show_bug.cgi?id=5308

I hope Squid will fix this on their side for future releases.

Greetings,
Matthias

Re: 7.3: Squid 6.3 with multiple workers - net.unix.dgram.sendspace too low

[Stuart Henderson]

On 2023/10/21 09:31, Silamael Darkomen wrote:
> On 20 Oct 2023 19:33, Stuart Henderson wrote:
> > After a few hours digging around, I eventually figured out where the
> > relevant sockets are created and have added a patch (to 7.4-stable and
> > -current) to bump buffers on them.
> 
> Hi Stuart,
> 
> Meanwhile I also did some digging and opened a bug at Squid with
> a patch attached:
> https://bugs.squid-cache.org/show_bug.cgi?id=5308
> I hope Squid will fix this on their side for future releases.
> 
> Greetings,
> Matthias
> 

I had another thought about this, we should probably fetch the
existing buffer size and avoid decreasing (in case the admin
already raised it system-wide)

relayd does not delete control socket on shutdown

[Kapetanakis Giannis]

After 7.4 relayd does not unlink it's socket

I've added the following but it's probably not enough. unveil?

G

Index: relayd.c
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
retrieving revision 1.191
diff -u -p -r1.191 relayd.c
--- relayd.c    25 Jun 2023 08:07:38 -    1.191
+++ relayd.c    21 Oct 2023 11:39:44 -
@@ -382,6 +382,8 @@ parent_shutdown(struct relayd *env)
 free(env->sc_ps);
 free(env);

+    unlink(env->sc_ps->ps_csock.cs_name);
+
 log_info("parent terminating, pid %d", getpid());

 exit(0);

Re: relayd does not delete control socket on shutdown

[Kapetanakis Giannis]

Rev 1.140 by florian@ seems to have changed that.

Do not try to unlink the control socket in an unprivileged child
process on shutdown.
Found while working ontame(2)  .
OK benno@

G


On 21/10/2023 14:41, Kapetanakis Giannis wrote:

After 7.4 relayd does not unlink it's socket

I've added the following but it's probably not enough. unveil?

G

Index: relayd.c
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
retrieving revision 1.191
diff -u -p -r1.191 relayd.c
--- relayd.c    25 Jun 2023 08:07:38 -    1.191
+++ relayd.c    21 Oct 2023 11:39:44 -
@@ -382,6 +382,8 @@ parent_shutdown(struct relayd *env)
 free(env->sc_ps);
 free(env);

+    unlink(env->sc_ps->ps_csock.cs_name);
+
 log_info("parent terminating, pid %d", getpid());

 exit(0);

Re: Prevent off-by-one accounting hang in out-of-swap situations

[Miod Vallat]

> Stuart, Miod, I wonder if this also help for the off-by-one issue you
> are seeing.  It might not.

It makes the aforementioned issue disappear on the affected machine.

> Comments, ok?

> diff --git sys/uvm/uvm_pdaemon.c sys/uvm/uvm_pdaemon.c
> index 284211d226c..a26a776df67 100644
> --- sys/uvm/uvm_pdaemon.c
> +++ sys/uvm/uvm_pdaemon.c

> @@ -917,9 +914,7 @@ uvmpd_scan(struct uvm_pmalloc *pma, struct 
> uvm_constraint_range *constraint)
>*/
>   free = uvmexp.free - BUFPAGES_DEFICIT;
>   swap_shortage = 0;
> - if (free < uvmexp.freetarg &&
> - uvmexp.swpginuse == uvmexp.swpages &&
> - !uvm_swapisfull() &&
> + if (free < uvmexp.freetarg && uvm_swapisfilled() && !uvm_swapisfull() &&
>   pages_freed == 0) {
>   swap_shortage = uvmexp.freetarg - free;
>   }

It's unfortunate that you now invoke two uvm_swapisxxx() routines, which
will both acquire a mutex. Maybe a third uvm_swapisxxx routine could be
introduced to compute the swapisfilled && !swapisfull condition at once?

Re: bt(5), btrace(8): execute END probe and print maps after exit() statement

[Martin Pieuchot]

On 18/10/23(Wed) 12:56, Scott Cheloha wrote:
> Hi,
> 
> A bt(5) exit() statement causes the btrace(8) interpreter to exit(3)
> immediately.
> 
> A BPFtrace exit() statement is more nuanced: the END probe is executed
> and the contents of all maps are printed before the interpreter exits.
> 
> This patch adds a halting check after the execution of each bt(5)
> statement.  If a statement causes the program to halt, the halt
> bubbles up to the top-level rule evaluation loop and terminates
> execution.  rules_teardown() then runs, just as if the program had
> received SIGTERM.
> 
> Two edge-like cases:
> 
> 1. You can exit() from BEGIN.  rules_setup() returns non-zero if this
>happens so the main loop knows to halt immediately.
> 
> 2. You can exit() from END.  This is just an early-return: the END probe
>doesn't run again.
> 
> Thoughts?

Makes sense to ease the transition from bpftrace scripts.  Ok with me if
you make sure the regression tests still pass.  Some outputs might
depend on the actual behavior and would need to be updated.

> 
> $ btrace -e '
> BEGIN {
>   @[probe] = "reached";
>   exit();
>   @[probe] = "not reached";
> }
> END {
>   @[probe] = "reached";
>   exit();
>   @[probe] = "not reached";
> }'
> 
> Index: btrace.c
> ===
> RCS file: /cvs/src/usr.sbin/btrace/btrace.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 btrace.c
> --- btrace.c  12 Oct 2023 15:16:44 -  1.79
> +++ btrace.c  18 Oct 2023 17:54:16 -
> @@ -71,10 +71,10 @@ struct dtioc_probe_info   *dtpi_get_by_val
>   * Main loop and rule evaluation.
>   */
>  void  rules_do(int);
> -void  rules_setup(int);
> -void  rules_apply(int, struct dt_evt *);
> +int   rules_setup(int);
> +int   rules_apply(int, struct dt_evt *);
>  void  rules_teardown(int);
> -void  rule_eval(struct bt_rule *, struct dt_evt *);
> +int   rule_eval(struct bt_rule *, struct dt_evt *);
>  void  rule_printmaps(struct bt_rule *);
>  
>  /*
> @@ -84,7 +84,7 @@ uint64_t builtin_nsecs(struct dt_evt *
>  const char   *builtin_kstack(struct dt_evt *);
>  const char   *builtin_arg(struct dt_evt *, enum bt_argtype);
>  struct bt_arg*fn_str(struct bt_arg *, struct dt_evt *, char 
> *);
> -void  stmt_eval(struct bt_stmt *, struct dt_evt *);
> +int   stmt_eval(struct bt_stmt *, struct dt_evt *);
>  void  stmt_bucketize(struct bt_stmt *, struct dt_evt *);
>  void  stmt_clear(struct bt_stmt *);
>  void  stmt_delete(struct bt_stmt *, struct dt_evt *);
> @@ -405,6 +405,7 @@ void
>  rules_do(int fd)
>  {
>   struct sigaction sa;
> + int halt = 0;
>  
>   memset(&sa, 0, sizeof(sa));
>   sigemptyset(&sa.sa_mask);
> @@ -415,9 +416,9 @@ rules_do(int fd)
>   if (sigaction(SIGTERM, &sa, NULL))
>   err(1, "sigaction");
>  
> - rules_setup(fd);
> + halt = rules_setup(fd);
>  
> - while (!quit_pending && g_nprobes > 0) {
> + while (!quit_pending && !halt && g_nprobes > 0) {
>   static struct dt_evt devtbuf[64];
>   ssize_t rlen;
>   size_t i;
> @@ -434,8 +435,11 @@ rules_do(int fd)
>   if ((rlen % sizeof(struct dt_evt)) != 0)
>   err(1, "incorrect read");
>  
> - for (i = 0; i < rlen / sizeof(struct dt_evt); i++)
> - rules_apply(fd, &devtbuf[i]);
> + for (i = 0; i < rlen / sizeof(struct dt_evt); i++) {
> + halt = rules_apply(fd, &devtbuf[i]);
> + if (halt)
> + break;
> + }
>   }
>  
>   rules_teardown(fd);
> @@ -484,7 +488,7 @@ rules_action_scan(struct bt_stmt *bs)
>   return evtflags;
>  }
>  
> -void
> +int
>  rules_setup(int fd)
>  {
>   struct dtioc_probe_info *dtpi;
> @@ -493,7 +497,7 @@ rules_setup(int fd)
>   struct bt_probe *bp;
>   struct bt_stmt *bs;
>   struct bt_arg *ba;
> - int dokstack = 0, on = 1;
> + int dokstack = 0, halt = 0, on = 1;
>   uint64_t evtflags;
>  
>   TAILQ_FOREACH(r, &g_rules, br_next) {
> @@ -553,7 +557,7 @@ rules_setup(int fd)
>   clock_gettime(CLOCK_REALTIME, &bt_devt.dtev_tsp);
>  
>   if (rbegin)
> - rule_eval(rbegin, &bt_devt);
> + halt = rule_eval(rbegin, &bt_devt);
>  
>   /* Enable all probes */
>   TAILQ_FOREACH(r, &g_rules, br_next) {
> @@ -571,9 +575,14 @@ rules_setup(int fd)
>   if (ioctl(fd, DTIOCRECORD, &on))
>   err(1, "DTIOCRECORD");
>   }
> +
> + return halt;
>  }
>  
> -void
> +/*
> + * Returns non-zero if the program should halt.
> + */
> +int
>  rules_apply(int fd, struct dt_evt *dtev)
>  {
>   struct bt_r

Re: Prevent off-by-one accounting hang in out-of-swap situations

[Martin Pieuchot]

On 21/10/23(Sat) 14:28, Miod Vallat wrote:
> > Stuart, Miod, I wonder if this also help for the off-by-one issue you
> > are seeing.  It might not.
> 
> It makes the aforementioned issue disappear on the affected machine.

Thanks at lot for testing!

> > Comments, ok?
> 
> > diff --git sys/uvm/uvm_pdaemon.c sys/uvm/uvm_pdaemon.c
> > index 284211d226c..a26a776df67 100644
> > --- sys/uvm/uvm_pdaemon.c
> > +++ sys/uvm/uvm_pdaemon.c
> 
> > @@ -917,9 +914,7 @@ uvmpd_scan(struct uvm_pmalloc *pma, struct 
> > uvm_constraint_range *constraint)
> >  */
> > free = uvmexp.free - BUFPAGES_DEFICIT;
> > swap_shortage = 0;
> > -   if (free < uvmexp.freetarg &&
> > -   uvmexp.swpginuse == uvmexp.swpages &&
> > -   !uvm_swapisfull() &&
> > +   if (free < uvmexp.freetarg && uvm_swapisfilled() && !uvm_swapisfull() &&
> > pages_freed == 0) {
> > swap_shortage = uvmexp.freetarg - free;
> > }
> 
> It's unfortunate that you now invoke two uvm_swapisxxx() routines, which
> will both acquire a mutex. Maybe a third uvm_swapisxxx routine could be
> introduced to compute the swapisfilled && !swapisfull condition at once?

I'm mot interested in such micro optimization yet.  Not acquiring a mutex
twice is IMHO not worth making half-shiny. 

However is someone is interested in going down in this direction, I'd
suggest try placing `uvmexp.freetarg' under the same lock and deal with
all its occurrences.  This is a possible next step to reduce the scope of
the uvm_lock_pageq() which is currently responsible for most of the
contention on MP in UVM.

Re: relayd does not delete control socket on shutdown

[Florian Obser]

On 2023-10-21 14:49 +03, Kapetanakis Giannis  wrote:
> Rev 1.140 by florian@ seems to have changed that.
>
> Do not try to unlink the control socket in an unprivileged child
> process on shutdown.
> Found while working ontame(2)  .
> OK benno@
>

Which was 8 years ago. I don't understand why you see a change in 7.4.

Anyway, we decided to not clean up control sockets in any of our
privsep daemons because leaving them behind does not cause any issues.

> G
>
>
> On 21/10/2023 14:41, Kapetanakis Giannis wrote:
>> After 7.4 relayd does not unlink it's socket
>>
>> I've added the following but it's probably not enough. unveil?
>>
>> G
>>
>> Index: relayd.c
>> ===
>> RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
>> retrieving revision 1.191
>> diff -u -p -r1.191 relayd.c
>> --- relayd.c    25 Jun 2023 08:07:38 -    1.191
>> +++ relayd.c    21 Oct 2023 11:39:44 -
>> @@ -382,6 +382,8 @@ parent_shutdown(struct relayd *env)
>>  free(env->sc_ps);
>>  free(env);
>>
>> +    unlink(env->sc_ps->ps_csock.cs_name);
>> +
>>  log_info("parent terminating, pid %d", getpid());
>>
>>  exit(0);
>>
>

-- 
In my defence, I have been left unsupervised.

Re: relayd does not delete control socket on shutdown

[Kapetanakis Giannis]

On 21/10/2023 20:39, Florian Obser wrote:

Which was 8 years ago. I don't understand why you see a change in 7.4.

Anyway, we decided to not clean up control sockets in any of our
privsep daemons because leaving them behind does not cause any issues.


I just noticed it today when I tried to use the socket in a script and 
noticed that it stayed there even after shutdown and though it was after 
7.4 but I was wrong about that.


Your commit made it that clear.

Agree it's not a big case if it stays there.

Would the unlink succeed if the socket was owned by _relayd?

G

Re: Virtio fix for testing

[Andrew Cagney]

On Mon, 21 Aug 2023 at 15:31, Andrew Cagney  wrote:
>
> On Sun, 20 Aug 2023 at 06:23, Stefan Fritsch  wrote:
> >
> > Am 13.08.23 um 17:38 schrieb Tobias Heider:

> > You could try something like
> >
> > -device virtio-scsi-pci,id=scsi
> > -drive file=install73.iso,format=raw,id=cdinst,if=none
> > -device scsi-cd,drive=cdinst
>
> good idea (just need to translate it to virt-install speak)
>
> > That depends on your seabios having support for virtio cdroms. Not sure
> > if that is the default by now.
> >
> > Or maybe try a SATA cdrom, but you would need to figure out the qemu
> > options for that yourself.

This is what I did.  Using virt-install and replacing --cdrom with
something like:

+KVM_OPENBSD_VIRT_INSTALL_FLAGS = \
+ --disk path=$(KVM_OPENBSD_BASE_ISO),readonly=on,device=cdrom,target.bus=sata \
+ --install bootdev=cdrom

I can boot/install 7.4's ISO.

thanks,
Andrew

Re: bt(5), btrace(8): execute END probe and print maps after exit() statement

[Scott Cheloha]

On Sat, Oct 21, 2023 at 07:17:05PM +0200, Martin Pieuchot wrote:
> On 18/10/23(Wed) 12:56, Scott Cheloha wrote:
> > Hi,
> > 
> > A bt(5) exit() statement causes the btrace(8) interpreter to exit(3)
> > immediately.
> > 
> > A BPFtrace exit() statement is more nuanced: the END probe is executed
> > and the contents of all maps are printed before the interpreter exits.
> > 
> > This patch adds a halting check after the execution of each bt(5)
> > statement.  If a statement causes the program to halt, the halt
> > bubbles up to the top-level rule evaluation loop and terminates
> > execution.  rules_teardown() then runs, just as if the program had
> > received SIGTERM.
> > 
> > Two edge-like cases:
> > 
> > 1. You can exit() from BEGIN.  rules_setup() returns non-zero if this
> >happens so the main loop knows to halt immediately.
> > 
> > 2. You can exit() from END.  This is just an early-return: the END probe
> >doesn't run again.
> > 
> > Thoughts?
> 
> Makes sense to ease the transition from bpftrace scripts.  Ok with me if
> you make sure the regression tests still pass.  Some outputs might
> depend on the actual behavior and would need to be updated.

Agh, my mistake, there are two tests that depend on the current
behavior.  I have updated them below.

ok with the test fixes?

Index: usr.sbin/btrace/btrace.c
===
RCS file: /cvs/src/usr.sbin/btrace/btrace.c,v
retrieving revision 1.79
diff -u -p -r1.79 btrace.c
--- usr.sbin/btrace/btrace.c12 Oct 2023 15:16:44 -  1.79
+++ usr.sbin/btrace/btrace.c22 Oct 2023 01:21:21 -
@@ -71,10 +71,10 @@ struct dtioc_probe_info *dtpi_get_by_val
  * Main loop and rule evaluation.
  */
 voidrules_do(int);
-voidrules_setup(int);
-voidrules_apply(int, struct dt_evt *);
+int rules_setup(int);
+int rules_apply(int, struct dt_evt *);
 voidrules_teardown(int);
-voidrule_eval(struct bt_rule *, struct dt_evt *);
+int rule_eval(struct bt_rule *, struct dt_evt *);
 voidrule_printmaps(struct bt_rule *);
 
 /*
@@ -84,7 +84,7 @@ uint64_t   builtin_nsecs(struct dt_evt *
 const char *builtin_kstack(struct dt_evt *);
 const char *builtin_arg(struct dt_evt *, enum bt_argtype);
 struct bt_arg  *fn_str(struct bt_arg *, struct dt_evt *, char *);
-voidstmt_eval(struct bt_stmt *, struct dt_evt *);
+int stmt_eval(struct bt_stmt *, struct dt_evt *);
 voidstmt_bucketize(struct bt_stmt *, struct dt_evt *);
 voidstmt_clear(struct bt_stmt *);
 voidstmt_delete(struct bt_stmt *, struct dt_evt *);
@@ -405,6 +405,7 @@ void
 rules_do(int fd)
 {
struct sigaction sa;
+   int halt = 0;
 
memset(&sa, 0, sizeof(sa));
sigemptyset(&sa.sa_mask);
@@ -415,9 +416,9 @@ rules_do(int fd)
if (sigaction(SIGTERM, &sa, NULL))
err(1, "sigaction");
 
-   rules_setup(fd);
+   halt = rules_setup(fd);
 
-   while (!quit_pending && g_nprobes > 0) {
+   while (!quit_pending && !halt && g_nprobes > 0) {
static struct dt_evt devtbuf[64];
ssize_t rlen;
size_t i;
@@ -434,8 +435,11 @@ rules_do(int fd)
if ((rlen % sizeof(struct dt_evt)) != 0)
err(1, "incorrect read");
 
-   for (i = 0; i < rlen / sizeof(struct dt_evt); i++)
-   rules_apply(fd, &devtbuf[i]);
+   for (i = 0; i < rlen / sizeof(struct dt_evt); i++) {
+   halt = rules_apply(fd, &devtbuf[i]);
+   if (halt)
+   break;
+   }
}
 
rules_teardown(fd);
@@ -484,7 +488,7 @@ rules_action_scan(struct bt_stmt *bs)
return evtflags;
 }
 
-void
+int
 rules_setup(int fd)
 {
struct dtioc_probe_info *dtpi;
@@ -493,7 +497,7 @@ rules_setup(int fd)
struct bt_probe *bp;
struct bt_stmt *bs;
struct bt_arg *ba;
-   int dokstack = 0, on = 1;
+   int dokstack = 0, halt = 0, on = 1;
uint64_t evtflags;
 
TAILQ_FOREACH(r, &g_rules, br_next) {
@@ -553,7 +557,7 @@ rules_setup(int fd)
clock_gettime(CLOCK_REALTIME, &bt_devt.dtev_tsp);
 
if (rbegin)
-   rule_eval(rbegin, &bt_devt);
+   halt = rule_eval(rbegin, &bt_devt);
 
/* Enable all probes */
TAILQ_FOREACH(r, &g_rules, br_next) {
@@ -571,9 +575,14 @@ rules_setup(int fd)
if (ioctl(fd, DTIOCRECORD, &on))
err(1, "DTIOCRECORD");
}
+
+   return halt;
 }
 
-void
+/*
+ * Returns non-zero if the program should halt.
+ */
+int
 rules_apply(int fd, struct dt_evt *dtev)
 {
struct bt_rule *r;