Improve error message in rcctl(8) again
Seven years ago I tried to restart a configuration file and it didn't
work out: https://marc.info/?l=openbsd-tech=147318006722787=2
This morning I tried to disable a different configuration file and got
similar results. Maybe my hands get too used to typing ".conf" when I
am fiddling with config files and restarting services. Or maybe I have
the mind of a LISP programmer, to whom code and data are the same
thing. But if I have not stopped passing config files to rcctl after
seven years I will probably never stop, so we should fix the error
message again.
Test cases:
# rcctl disable foo.bar
# rcctl set foo.bar status off
Compare the error messages before and after the patch.
diff --git usr.sbin/rcctl/rcctl.sh usr.sbin/rcctl/rcctl.sh
index fb87943ba00..489c0217c45 100644
--- usr.sbin/rcctl/rcctl.sh
+++ usr.sbin/rcctl/rcctl.sh
@@ -541,6 +541,12 @@ case ${action} in
svc_is_avail ${svc} || \
rcctl_err "service ${svc} does not
exist" 2
done
+ else
+ # But you still have to catch invalid characters
+ for svc in ${svcs}; do
+ _rc_check_name "${svc}" || \
+ rcctl_err "service ${svc} does not
exist" 2
+ done
fi
;;
get|getdef)
@@ -572,6 +578,9 @@ case ${action} in
if [ "${action} ${var} ${args}" != "set status off" ]; then
svc_is_avail ${svc} || \
rcctl_err "service ${svc} does not exist" 2
+ else
+ _rc_check_name "${svc}" || \
+ rcctl_err "service ${svc} does not exist" 2
fi
[[ ${var} !=
@(class|execdir|flags|logger|rtable|status|timeout|user) ]] && usage
svc_is_meta ${svc} && [ "${var}" != "status" ] && \
Re: OpenBSD::MkTemp vs Devel::Cover
Tl;Dr: I'm not trying too hard to use Devel::Cover but so far it fails abysmally with BOTH pkg_add and dpb. I'll admit that both are playing "fun" games with identity but seriously, Devel::NYTProf didn't care at all ! I've spent a few hours trying to figure out options (loose_perms won't help) Knowing how much of my perl is actually used would be a HUGE HELP in crafting more non regression tests, but so far, Devel::Cover lives up to its "alpha for 20 years" reputation. Triggering the problems is not hard: pkg_add breaks, dpb breaks even earlier...
pkg_*: the road forward
I spent some time during the last hackathon, talking to various people over where we're going. I'm a bit afraid that people are going to see the ports/package framework as "not invited" (because some of the subsystems of OpenBSD are not exactly oustide friendly) Right now, I got one student directly working with me on the tools, and I must say it is a good thing: I have wy too many idiosyncrasies. And I'm trying to get better at documenting ?` So where are we going ? there are lots of pieces missing, some of them trivial, some really annoying. - the level of testing is abysmal. I would really like for someone to have fun with regress and make it manageable. Right now, it's a pain ! More tooling to make pkg_add "fully" regression testable would be very helpful. - likewise register-plist needs some tests. I have made some progress in options to make that happen. - I would love to get some Devel::Cover stats... zero progress about this during this week (well, the main progress was that it does not work) - I converted ALL my code to perl 5.36. Having signatures is a HUGE change, not wrt actual code, but with readability. It makes perl code look like "other" programming languages, in a good way, thus making it sooo much easier for new people to get in. As for the actual code changes: - we need more code to deal with "global state", mainly /etc/rc changes and messages. Those files are going to move around, which means it's going to be GLOBAL state, because it can move from package to package. I would really really love to have regress test cases first. - we still need better Vstat code that deals with symlinks. There's one test that actually fails... and writing more tests should be simpler. - the code dealing with "old libs" is still incredibly ad-hock and needing some love. - there's something fishy in pkg_create wrt dependency handling... figure it out, AND make it work with tags as well. - fragments building is oversimplified, that's the main thing holding back ocaml "smart fragments". - texlive wants some love. That one is pretty much mine: a bit of targets in bsd.port.mk, a bit of "hints" code in update-plist, and also the ability to say "oh, don't package this, it's already in". Nothing really difficult, it's just that texlive is large... - displays and codes from pkg_add/pkg_info. The main culprit right now is FETCH_PACKAGES, it could be better. - "libsets". Updating wantlibs is really a pain for big changes. I have a keyword called libset to deal with that. It requires changes to bsd.port.mk and to pkg_add. Simply put: a libset would have a version and say "we need those libs to work". If a libset got bumped, then we wouldn't say the affected libs are a problem if they change ! This is mostly a question of writing the code. - lib-depends-check: the script AND its dependent libraries need a lot of clean-up, so that an average person can understand them. Then we can deal (probably) with subdirectories (which we don't) - dpb changes: the "files needed" change. I have all the pieces to figure out which files and directories a given build requires: in the ports tree, MAKEFILES_LIST + PKGDIR + PATCHDIR + FILESDIR + package + distfiles would be enough. I need to actually provide this. This would enable TWO huge things: -> disconnected builds, with a cluster that doesn't use nfs, but runs openrsync instead -> separate builds into separate chroots (roughly 6000+ links per build) - regress tests: should be much easier now that diskspace is (mostly) not an issue - roaching while fetching: the main part would be to figure out which part of roach we actually need to plug in after fetch. Having a sqlite database is actually a huge hindrance. dpb could output "decent" logs, and it should be easy to coerce them. I probably missed some entries, but this is what occupies my ports/pkg* thoughts. Help welcome. Not perl solutions NOT welcome.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
On Mon, Jul 10, 2023 at 05:19:35PM +0200, Mark Kettenis wrote: > > Date: Mon, 10 Jul 2023 09:57:39 -0500 > > From: Scott Cheloha > > > > On Mon, Jul 10, 2023 at 07:42:55AM -0600, Theo de Raadt wrote: > > > I dare you to write the simplest fix for this, instead of a diff that > > > scrolls by. > > > > This patch seems to work. Will need to bang on it for a few more days. > > > > 1. Disable gmoninit after sched_stop_scondary_cpus(). The secondary > >CPUs have halted, so we aren't racing sysctl(2) on a secondary CPU. > > > > 2. Restore gmoninit between resume_mp() and sched_start_secondary_cpus(). > >The secondary CPUs are out of cpu_hatch(), which is probably where we > >are crashing during resume. The secondary CPUs haven't started > >scheduling yet, so we aren't racing sysctl(2). > > It is still a bit scary to have cpu_hatch() call _mcount() but I guess > adding __attribute__((no_profile)) to all of the functions called by > cpu_hatch() isn't really workable either. > > That said... > > > Index: subr_suspend.c > > === > > RCS file: /cvs/src/sys/kern/subr_suspend.c,v > > retrieving revision 1.15 > > diff -u -p -r1.15 subr_suspend.c > > --- subr_suspend.c 2 Jul 2023 19:02:27 - 1.15 > > +++ subr_suspend.c 10 Jul 2023 14:51:01 - > > @@ -18,6 +18,7 @@ > > > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -100,6 +101,14 @@ top: > > #ifdef MULTIPROCESSOR > > sched_stop_secondary_cpus(); > > KASSERT(CPU_IS_PRIMARY(curcpu())); > > +#endif > > +#ifdef GPROF > > + extern int gmoninit; > > + int gmon_state = gmoninit; > > No variable declarations in the middle of functions please. Yep, moved up. > > + gmoninit = 0; > > + membar_producer(); > > Why are you messing with memory barriers here? > > > +#endif > > +#ifdef MULTIPROCESSOR > > sleep_mp(); > > #endif > > > > @@ -172,6 +181,12 @@ fail_suspend: > > resume_randomness(rndbuf, rndbuflen); > > #ifdef MULTIPROCESSOR > > resume_mp(); > > +#endif > > +#ifdef GPROF > > + gmoninit = gmon_state; > > + membar_producer(); > > And here? gmoninit is not volatile. I had this idea that I wanted the store to be globally visible before proceeding. But you and Theo are telling me I don't need the barrier. I'll take your word for it, dropped. -- suspend/resume and hibernate/unhibernate still work with the revised patch below. Index: subr_suspend.c === RCS file: /cvs/src/sys/kern/subr_suspend.c,v retrieving revision 1.15 diff -u -p -r1.15 subr_suspend.c --- subr_suspend.c 2 Jul 2023 19:02:27 - 1.15 +++ subr_suspend.c 10 Jul 2023 15:30:22 - @@ -26,6 +26,9 @@ #include #include #include +#ifdef GPROF +#include +#endif #ifdef HIBERNATE #include #endif @@ -49,6 +52,9 @@ sleep_state(void *v, int sleepmode) extern int perflevel; size_t rndbuflen; char *rndbuf; +#ifdef GPROF + int gmon_state; +#endif #if NSOFTRAID > 0 extern void sr_quiesce(void); #endif @@ -100,6 +106,12 @@ top: #ifdef MULTIPROCESSOR sched_stop_secondary_cpus(); KASSERT(CPU_IS_PRIMARY(curcpu())); +#endif +#ifdef GPROF + gmon_state = gmoninit; + gmoninit = 0; +#endif +#ifdef MULTIPROCESSOR sleep_mp(); #endif @@ -172,6 +184,11 @@ fail_suspend: resume_randomness(rndbuf, rndbuflen); #ifdef MULTIPROCESSOR resume_mp(); +#endif +#ifdef GPROF + gmoninit = gmon_state; +#endif +#ifdef MULTIPROCESSOR sched_start_secondary_cpus(); #endif vfs_stall(curproc, 0);
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
Mark Kettenis wrote: > It is still a bit scary to have cpu_hatch() call _mcount() but I guess > adding __attribute__((no_profile)) to all of the functions called by > cpu_hatch() isn't really workable either. It now immediately returns. > > + int gmon_state = gmoninit; > > No variable declarations in the middle of functions please. Yep. Put it at the top. > > + gmoninit = 0; > > + membar_producer(); > > Why are you messing with memory barriers here? I have asked the same question. This has potential to make things worse.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
> Date: Mon, 10 Jul 2023 09:57:39 -0500 > From: Scott Cheloha > > On Mon, Jul 10, 2023 at 07:42:55AM -0600, Theo de Raadt wrote: > > I dare you to write the simplest fix for this, instead of a diff that > > scrolls by. > > This patch seems to work. Will need to bang on it for a few more days. > > 1. Disable gmoninit after sched_stop_scondary_cpus(). The secondary >CPUs have halted, so we aren't racing sysctl(2) on a secondary CPU. > > 2. Restore gmoninit between resume_mp() and sched_start_secondary_cpus(). >The secondary CPUs are out of cpu_hatch(), which is probably where we >are crashing during resume. The secondary CPUs haven't started >scheduling yet, so we aren't racing sysctl(2). It is still a bit scary to have cpu_hatch() call _mcount() but I guess adding __attribute__((no_profile)) to all of the functions called by cpu_hatch() isn't really workable either. That said... > Index: subr_suspend.c > === > RCS file: /cvs/src/sys/kern/subr_suspend.c,v > retrieving revision 1.15 > diff -u -p -r1.15 subr_suspend.c > --- subr_suspend.c2 Jul 2023 19:02:27 - 1.15 > +++ subr_suspend.c10 Jul 2023 14:51:01 - > @@ -18,6 +18,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -100,6 +101,14 @@ top: > #ifdef MULTIPROCESSOR > sched_stop_secondary_cpus(); > KASSERT(CPU_IS_PRIMARY(curcpu())); > +#endif > +#ifdef GPROF > + extern int gmoninit; > + int gmon_state = gmoninit; No variable declarations in the middle of functions please. > + gmoninit = 0; > + membar_producer(); Why are you messing with memory barriers here? > +#endif > +#ifdef MULTIPROCESSOR > sleep_mp(); > #endif > > @@ -172,6 +181,12 @@ fail_suspend: > resume_randomness(rndbuf, rndbuflen); > #ifdef MULTIPROCESSOR > resume_mp(); > +#endif > +#ifdef GPROF > + gmoninit = gmon_state; > + membar_producer(); And here? > +#endif > +#ifdef MULTIPROCESSOR > sched_start_secondary_cpus(); > #endif > vfs_stall(curproc, 0); >
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
On Mon, Jul 10, 2023 at 07:42:55AM -0600, Theo de Raadt wrote: > I dare you to write the simplest fix for this, instead of a diff that > scrolls by. This patch seems to work. Will need to bang on it for a few more days. 1. Disable gmoninit after sched_stop_scondary_cpus(). The secondary CPUs have halted, so we aren't racing sysctl(2) on a secondary CPU. 2. Restore gmoninit between resume_mp() and sched_start_secondary_cpus(). The secondary CPUs are out of cpu_hatch(), which is probably where we are crashing during resume. The secondary CPUs haven't started scheduling yet, so we aren't racing sysctl(2). Index: subr_suspend.c === RCS file: /cvs/src/sys/kern/subr_suspend.c,v retrieving revision 1.15 diff -u -p -r1.15 subr_suspend.c --- subr_suspend.c 2 Jul 2023 19:02:27 - 1.15 +++ subr_suspend.c 10 Jul 2023 14:51:01 - @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -100,6 +101,14 @@ top: #ifdef MULTIPROCESSOR sched_stop_secondary_cpus(); KASSERT(CPU_IS_PRIMARY(curcpu())); +#endif +#ifdef GPROF + extern int gmoninit; + int gmon_state = gmoninit; + gmoninit = 0; + membar_producer(); +#endif +#ifdef MULTIPROCESSOR sleep_mp(); #endif @@ -172,6 +181,12 @@ fail_suspend: resume_randomness(rndbuf, rndbuflen); #ifdef MULTIPROCESSOR resume_mp(); +#endif +#ifdef GPROF + gmoninit = gmon_state; + membar_producer(); +#endif +#ifdef MULTIPROCESSOR sched_start_secondary_cpus(); #endif vfs_stall(curproc, 0);
Re: [PATCH] Support PS2 keyboard on chrromebook
Ping? Le lun. 3 juil. 2023, 02:59, Vladimir 'phcoder' Serbinenko < phco...@gmail.com> a écrit : > On, at least, some Chromebook PS/2 protocol is implemented by EC rather > than a real PS/2 controller. It works fine except for 2 things: > * Unusual layout like multimedia keys instead of F* > * Reset command returns garbage (usually last key) > This patch attempts to handle later as it stops keyboard from being > recognized at all. It works by checking getid if reset fails. How other > OSes handle the situation: > * Windows: no idea, but keyboard works > * Linux. Check only getid results. Reset is done but it's results are > ignored. > * FreeBSD. Skips probing if firmware is determined to be coreboot and > assumes presence of PS/2 keyboard. This is wrong e.g. coreboot supports > some MacBooks and they use USB keyboard instead. > * NetBSD shares the same code as OpenBSD AFAICT. Probably broken. > * Haiku was broken and recently they accepted a similar path from me. Ref: > https://review.haiku-os.org/c/haiku/+/6610 > >
Re: [PATCH] Implement ext2 incompat feature 64-bit
Ping? Le lun. 3 juil. 2023, 02:49, Vladimir 'phcoder' Serbinenko < phco...@gmail.com> a écrit : > Hello, all attached patch implements feature 64-bit for ext2. This was > enabled implicitly on my Ubuntu and probably on many other systems. Since > it's an incompat feature lack of its support prevented the mount > altogether. With this patch I was able to load install sets from my Ubuntu > partition. >
relayd redirect does not stay down for disabled table
Hello,
I have a problem with relayd and redirects. If I disable a table, redirect
stays down only for a while.
After a few seconds, redirect gets active again and forwards to the disabled
table.
Same happens for redirect with a backup forward table.
Redirect points momentarily to backup table but after a while forwards to the
disabled table.
This happens only with a combination of a table with parent hosts.
patch at bottom
regards,
Giannis
table { dir1 retry 2, dir2 retry 2 }
table { dir1 parent 1 retry 2, dir2 parent 2 retry 2 }
table { foo1 retry 2, foo2 retry 2 }
redirect dir-imap {
listen on $dir_addr port imaps
pftag RELAYD_dir
sticky-address
forward to port 993 mode least-states check icmp
}
redirect dir-pop {
listen on $dir_addr port pop3s
pftag RELAYD_dir
sticky-address
forward to port 995 mode least-states check icmp
}
redirect dir-lmtp {
listen on $dir_addr port 24
pftag RELAYD_dir
sticky-address
forward to port 24 mode least-states check icmp
forward to port 24 mode least-states check icmp
}
# relayctl show sum
Id TypeNameAvlblty Status
1 redirectdir-imapactive
1 table dir:993 active (2 hosts)
1 hostdir1100.00% up
2 hostdir2100.00% up
2 redirectdir-pop active
2 table dir_:995active (2 hosts)
3 hostdir1 parent 1 100.00% up
4 hostdir2 parent 2 100.00% up
3 redirectdir-lmtpactive
3 table dir_:24 active (2 hosts)
5 hostdir1 parent 1 100.00% up
6 hostdir2 parent 2 100.00% up
4 table dir_backup:24 active (2 hosts)
7 hostfoo1100.00% up
8 hostfoo2100.00% up
# relayctl table dis dir_:995
disable_table: table 2
flush_table: flushed table dir-pop
pfe_sync: disabling ruleset
sync_ruleset: rules removed
pfe_dispatch_hce: state 1 for host 4 dir2
pfe_dispatch_hce: state 1 for host 3 dir1
# relayctl show sum
Id TypeNameAvlblty Status
2 redirectdir-pop down
2 table dir_:995disabled
# pfctl -a 'relayd/*' -sr
anchor "dir-pop" all {
} // empty as it should
But after a while:
table dir-pop: 2 added, 0 deleted, 0 changed, 0 killed
pfe_sync: enabling ruleset
sync_ruleset: rule added to anchor "relayd/dir-pop"
# relayctl show sum
Id TypeNameAvlblty Status
2 redirectdir-pop active
2 table dir_:995disabled
Although table is disabled, redirect comes active,
pf rule in anchor is active and table has dir1 and dir2 inside.
# pfctl -a 'relayd/*' -sr
anchor "dir-pop" all {
pass in quick on rdomain 0 inet proto tcp from any to $dir_addr port = 995
flags S/SA keep state (tcp.established 600) tag RELAYD_dir rdr-to
port 995 least-states sticky-address
}
Same happens with the backup table on last dir-lmtp redirect.
Table is updated momentarily with the backup hosts,
but after a while traffic is forwarded back to primary hosts although their
table is disabled.
# relayctl show sum
Id TypeNameAvlblty Status
3 redirectdir-lmtpactive
3 table dir_:24 active (2 hosts)
5 hostdir1 parent 1 100.00% up
6 hostdir2 parent 2 100.00% up
4 table dir_backup:24 active (2 hosts)
7 hostfoo1100.00% up
8 hostfoo2100.00% up
# relayctl table dis dir_:24
disable_table: table 3
table dir-lmtp: 2 added, 2 deleted, 0 changed, 0 killed
pfe_dispatch_hce: state 1 for host 6 dir2
pfe_dispatch_hce: state 1 for host 5 dir1
# relayctl show sum
Id TypeNameAvlblty Status
3 redirectdir-lmtpactive (using
backup table)
3 table dir_:24 disabled
4 table dir_backup:24 active (2 hosts)
7 hostfoo1100.00% up
8 hostfoo2100.00%
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
I dare you to write the simplest fix for this, instead of a diff that scrolls by.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
Scott Cheloha wrote: > Secondary CPUs are still running at the top of sleep_state(). To > disable _mcount with gmoninit we would need to wait until after > secondary CPUs have halted to toggle it off, which is way further into > sleep_state(). I suspect you are exaggerating the window of time when _mcount is dangerous to call. Disable it early, re-enable it late, and avoid trying to write a complicated solution.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
On Mon, Jul 10, 2023 at 07:09:19AM -0600, Theo de Raadt wrote: > Mark Kettenis wrote: > > > So isn't the real problem that some of the lower-level code involved > > in the resume path isn't properly marked to not do the > > instrumentation? Traditionally that was assembly code and we'd use > > NENTRY() (in amd64) or ENTRY_NP() (on some other architectures) to > > prevent thise functions from calling _mcount(). But that was only > > ever done for code used during early bootstrap of the kernel. And > > these days there may be C code that needs this as well. > > > > With your diff, functions in the suspend/resume path will still call > > _mcount() which may not be safe. > > I guess you can make critical functions not do _PROF_PROLOGUE > or you can make __mcount or _mcount aware that they should "do nothing", > or "nothing scary". > > Hell, save & toggle the 'gmoninit' variable during the suspend/resume > sequence, and then adjust one comment: > > /* > * Do not profile execution if memory for the current CPU > * descriptor and profiling buffers has not yet been allocated > * or if the CPU we are running on has not yet set its trap > -* handler > +* handler, or disabled during a suspend/resume sequence > */ > if (gmoninit == 0) > return; > > > Does this really need another variable? > > It feels like this can be 4 1-line diffs. Secondary CPUs are still running at the top of sleep_state(). To disable _mcount with gmoninit we would need to wait until after secondary CPUs have halted to toggle it off, which is way further into sleep_state(). Then, on the resume side, you need to keep the secondary CPUs from toggling gmoninit back on until after all other secondary CPUs have finished restarting, which I think means changing how we do sched_start_secondary_cpus(). ... given all of that, I thought adding a second variable was easier and less likely to break something more important than GPROF.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
Mark Kettenis wrote: > So isn't the real problem that some of the lower-level code involved > in the resume path isn't properly marked to not do the > instrumentation? Traditionally that was assembly code and we'd use > NENTRY() (in amd64) or ENTRY_NP() (on some other architectures) to > prevent thise functions from calling _mcount(). But that was only > ever done for code used during early bootstrap of the kernel. And > these days there may be C code that needs this as well. > > With your diff, functions in the suspend/resume path will still call > _mcount() which may not be safe. I guess you can make critical functions not do _PROF_PROLOGUE or you can make __mcount or _mcount aware that they should "do nothing", or "nothing scary". Hell, save & toggle the 'gmoninit' variable during the suspend/resume sequence, and then adjust one comment: /* * Do not profile execution if memory for the current CPU * descriptor and profiling buffers has not yet been allocated * or if the CPU we are running on has not yet set its trap -* handler +* handler, or disabled during a suspend/resume sequence */ if (gmoninit == 0) return; Does this really need another variable? It feels like this can be 4 1-line diffs.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
> Date: Sun, 9 Jul 2023 17:24:41 -0500 > From: Scott Cheloha > > On Sun, Jul 09, 2023 at 08:11:43PM +0200, Claudio Jeker wrote: > > On Sun, Jul 09, 2023 at 12:52:20PM -0500, Scott Cheloha wrote: > > > This patch fixes resume/unhibernate on GPROF kernels where kgmon(8) > > > has activated kernel profiling. > > > > > > I think the problem is that code called from cpu_hatch() does not play > > > nicely with _mcount(), so GPROF kernels crash during resume. I can't > > > point you to which code in particular, but keeping all CPUs out of > > > _mcount() until the primary CPU has completed resume/unhibernate fixes > > > the crash. > > > > > > ok? > > > > To be honest, I'm not sure we need something like this. GPROF is already a > > special case and poeple running a GPROF kernel should probably stop the > > collection of profile data before suspend/hibernate. > > Sorry, I was a little unclear in my original mail. > > When I say "has activated kernel profiling" I mean "has *ever* > activated kernel profiling". > > Regardless of whether or not profiling is active at the moment we > reach sleep_state(), if kernel profiling has *ever* been activated in > the past, the resume crashes. So isn't the real problem that some of the lower-level code involved in the resume path isn't properly marked to not do the instrumentation? Traditionally that was assembly code and we'd use NENTRY() (in amd64) or ENTRY_NP() (on some other architectures) to prevent thise functions from calling _mcount(). But that was only ever done for code used during early bootstrap of the kernel. And these days there may be C code that needs this as well. With your diff, functions in the suspend/resume path will still call _mcount() which may not be safe.
Re: pf.os database /p0f
On 3 Jul 2023, at 23:39, Lee, Jonathan D wrote: > [cid:cd2efd41-42cb-4d83-9173-521bbb8f4539@namprd04.prod.outlook.com] > > Hello fellow software developers, > > I have noticed that p0f database files are not being updated. And I have noticed that you sent two copies of a 6.7 megabyte email to a mailing list which rarely sees emails which are over 100-kilobytes. And by pure coincidence, my mail quota was exceeded shortly after those two emails arrived. (although it wasn't until today that it occurred to me to check the size of the emails in my mailbox) This was very annoying. -- garance alistair drosehn leed developer @RPI
Re: tcp lro by default, call for testing
On Sat, Jul 08, 2023 at 05:15:26PM +0300, Alexander Bluhm wrote:
> I am not aware of any more limitations when enabling LRO for TCP
> in the network drivers. The feature allows to receive agregated
> packets larger than the MTU. Receiving TCP streams becomes much
> faster.
>
> As the network hardware is not aware whether a packet is received
> locally or forwarded, everything is aggregated. In case of forwarding
> it is split on output to packets not larger than the original
> packets. So path MTU discovery should still work. If the outgoing
> interface supports TSO, the packet is chopped in hardware.
>
> Currently only ix(4) and lo(4) support LRO, and ix(4) is limited
> to IPv4 and newer than the old 82598 model. If the interface is
> added to a bridge(4) or aggr(4), LRO is automatically disabled.
I guess you mean veb(4) not aggr(4). We just avoid the in heritage
of the LRO capability in aggr(4) but are using the feature.
> So in case you possess any ix(4) hardware or do funky pf routing
> on lo(4) please run this diff. If you encounter problems, report
> and turn LRO off per interface with ifconfig -tcplro.
Diff looks fine to me. I just would keep mentioning the default
behavior in the manpage like this:
ok jan@
Index: sbin/ifconfig/ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.397
diff -u -p -r1.397 ifconfig.8
--- sbin/ifconfig/ifconfig.87 Jun 2023 18:42:40 - 1.397
+++ sbin/ifconfig/ifconfig.810 Jul 2023 11:54:47 -
@@ -517,9 +517,9 @@ It is not possible to use LRO with inter
or
.Xr tpmr 4 .
Changing this option will re-initialize the network interface.
+LRO is enabled by default.
.It Cm -tcplro
Disable LRO.
-LRO is disabled by default.
.It Cm up
Mark an interface
.Dq up .
> Index: sys/dev/pci/if_ix.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_ix.c,v
> retrieving revision 1.198
> diff -u -p -r1.198 if_ix.c
> --- sys/dev/pci/if_ix.c 8 Jul 2023 09:01:30 - 1.198
> +++ sys/dev/pci/if_ix.c 8 Jul 2023 13:51:26 -
> @@ -1925,8 +1925,10 @@ ixgbe_setup_interface(struct ix_softc *s
> ifp->if_capabilities |= IFCAP_CSUM_IPv4;
>
> ifp->if_capabilities |= IFCAP_TSOv4 | IFCAP_TSOv6;
> - if (sc->hw.mac.type != ixgbe_mac_82598EB)
> + if (sc->hw.mac.type != ixgbe_mac_82598EB) {
> + ifp->if_xflags |= IFXF_LRO;
> ifp->if_capabilities |= IFCAP_LRO;
> + }
>
> /*
>* Specify the media types supported by this sc and register
> Index: sys/net/if_loop.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_loop.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 if_loop.c
> --- sys/net/if_loop.c 2 Jul 2023 19:59:15 - 1.95
> +++ sys/net/if_loop.c 8 Jul 2023 13:51:26 -
> @@ -172,11 +172,11 @@ loop_clone_create(struct if_clone *ifc,
> ifp->if_softc = NULL;
> ifp->if_mtu = LOMTU;
> ifp->if_flags = IFF_LOOPBACK | IFF_MULTICAST;
> - ifp->if_xflags = IFXF_CLONED;
> + ifp->if_xflags = IFXF_CLONED | IFXF_LRO;
> ifp->if_capabilities = IFCAP_CSUM_IPv4 |
> IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4 |
> IFCAP_CSUM_TCPv6 | IFCAP_CSUM_UDPv6 |
> - IFCAP_LRO;
> + IFCAP_LRO | IFCAP_TSOv4 | IFCAP_TSOv6;
> ifp->if_rtrequest = lortrequest;
> ifp->if_ioctl = loioctl;
> ifp->if_input = loinput;
> Index: sbin/ifconfig/ifconfig.8
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.397
> diff -u -p -r1.397 ifconfig.8
> --- sbin/ifconfig/ifconfig.8 7 Jun 2023 18:42:40 - 1.397
> +++ sbin/ifconfig/ifconfig.8 7 Jul 2023 19:57:09 -
> @@ -519,7 +519,6 @@ or
> Changing this option will re-initialize the network interface.
> .It Cm -tcplro
> Disable LRO.
> -LRO is disabled by default.
> .It Cm up
> Mark an interface
> .Dq up .
>
