Re: svn commit: r365449 - head/sbin/rcorder
Van: John Baldwin Datum: vrijdag, 18 september 2020 00:50 Aan: "Olivier Cochard-Labbé" CC: "Andrey V. Elsukov" , src-committers , svn-src-all , svn-src-head Onderwerp: Re: svn commit: r365449 - head/sbin/rcorder On 9/17/20 3:44 PM, Olivier Cochard-Labbé wrote: > On Thu, Sep 17, 2020 at 10:21 PM John Baldwin wrote: >> I think long term we want OCF's notions of sessions to be a bit more >> fluid such that "client" sessions for things like GELI and IPSec can >> be backed by one or more "driver" sessions (including "driver" sessions >> coming and going as devices come and go). That's a fair bit more work >> however. >> >> > And why not simply add 'kld' into the REQUIRE part of /etc/rc.d/ipsec ? > But this will fix only IPsec: What about other crypto consumers ? The problem is that kld_list can be used to load all sorts of modules. Perhaps some of them need to be loaded after the network is configured, for example, but ipsec is probably before NETWORKING, so moving kld earlier would break those modules. The problem with 'kld' is that it is super generic so there isn't a "right" place to put it. I'm not sure of an easy solution. In your case, if you put aesni_load=YES in loader.conf instead of putting aesni in kld_list you would be fine though. I'm not sure it's really safe to assume that things loaded by kld_list are available for any boot-time services. -- John Baldwin ___ svn-src-...@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org" Another solution could be to create a /etc/rc.d/aesni script which loads the aesni module and use that for dependencies instead of the generic kld_list. Or a more generic /etc/rc.d/cryptostuff. Regards, Ronald. ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r365449 - head/sbin/rcorder
On 9/17/20 3:44 PM, Olivier Cochard-Labbé wrote: > On Thu, Sep 17, 2020 at 10:21 PM John Baldwin wrote: >> I think long term we want OCF's notions of sessions to be a bit more >> fluid such that "client" sessions for things like GELI and IPSec can >> be backed by one or more "driver" sessions (including "driver" sessions >> coming and going as devices come and go). That's a fair bit more work >> however. >> >> > And why not simply add 'kld' into the REQUIRE part of /etc/rc.d/ipsec ? > But this will fix only IPsec: What about other crypto consumers ? The problem is that kld_list can be used to load all sorts of modules. Perhaps some of them need to be loaded after the network is configured, for example, but ipsec is probably before NETWORKING, so moving kld earlier would break those modules. The problem with 'kld' is that it is super generic so there isn't a "right" place to put it. I'm not sure of an easy solution. In your case, if you put aesni_load=YES in loader.conf instead of putting aesni in kld_list you would be fine though. I'm not sure it's really safe to assume that things loaded by kld_list are available for any boot-time services. -- John Baldwin ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r365449 - head/sbin/rcorder
On Thu, Sep 17, 2020 at 10:21 PM John Baldwin wrote: > > > I don't think the issue is with rcorder though. I think the reason the > ordering matters warrants further investigation. Is aesni not getting > used when ipsec is loaded first? You can use dtrace with the script at > https://github.com/bsdjhb/kdbg/blob/master/dtrace/crypto_drivers.d to see > which driver is being used. > > Hi, The driver used is cryptosoft when ipsec keys are set before the aesni module is loaded. I suspect > btw that you could just do 'sh /etc/rc.d/ipsec restart' post-boot without > unloading any modules which would also fix your benchmark. > Correct, just restarting ipsec fix the benchmark. And once restarted, the driver used is aesni. > I think long term we want OCF's notions of sessions to be a bit more > fluid such that "client" sessions for things like GELI and IPSec can > be backed by one or more "driver" sessions (including "driver" sessions > coming and going as devices come and go). That's a fair bit more work > however. > > And why not simply add 'kld' into the REQUIRE part of /etc/rc.d/ipsec ? But this will fix only IPsec: What about other crypto consumers ? Regards, Olivier ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r365449 - head/sbin/rcorder
On 9/17/20 10:49 AM, Olivier Cochard-Labbé wrote: > On Tue, Sep 8, 2020 at 12:36 PM Andrey V. Elsukov wrote: > >> Author: ae >> Date: Tue Sep 8 10:36:11 2020 >> New Revision: 365449 >> URL: https://svnweb.freebsd.org/changeset/base/365449 >> >> Log: >> Add a few features to rcorder: >> >> >> > Hi Andrey, > > I've spent some time bisecting an IPSec gateway performance regression on > -head that points to this commit. > > So my bench uses a simple static IPSec aes-gcm-16 setup, and their results > vary a lot between those 2 revisions: > >- r365448: Estimated Equilibrium Ethernet throughput= 1413 Mb/s (maximum >value seen: 1419 Mb/s) >- r365449: Estimated Equilibrium Ethernet throughput= 469 Mb/s (maximum >value seen: 469 Mb/s) > > How could a modification to the rcoder be the source cause of that ? > My rc.conf contains this: > kld_list="aesni" > ipsec_enable="YES" > > So My first thought was that the aesni module wasn't loaded anymore: > Before upgrade: > [root@hp]~# uname -a > FreeBSD hp 13.0-CURRENT FreeBSD 13.0-CURRENT #15 r365448M: Thu Sep 17 > 18:17:58 CEST 2020 oliv...@lame4.bsdrp.net:/usr/src/amd64.amd64/sys/amd64 > amd64 > [root@hp]~# kldstat > Id Refs AddressSize Name > 1 11 0x8020 1ee58b0 kernel > 21 0x82319000 34c8 fdescfs.ko > 31 0x8231d000 a240 aesni.ko > 41 0x82328000 8c98 ioat.ko > 51 0x82331000 e350 ipsec.ko > > Then after upgrade: > > [root@hp]~# uname -a > FreeBSD hp 13.0-CURRENT FreeBSD 13.0-CURRENT #14 r365449M: Thu Sep 17 > 17:01:35 CEST 2020 oliv...@lame4.bsdrp.net:/usr/src/amd64.amd64/sys/amd64 > amd64 > [root@hp]~# kldstat > Id Refs AddressSize Name > 1 11 0x8020 1ee58b0 kernel > 21 0x82319000 34c8 fdescfs.ko > 31 0x8231d000 e350 ipsec.ko > 41 0x8232c000 a240 aesni.ko > 51 0x82337000 8c98 ioat.ko > > => aesni.ko is correctly loaded, so it is not the problem, but notice the > order of the kernel modules that have changed. > Could be this the source of the problem ? Let's try: > > [root@hp]~# service ipsec stop > Clearing ipsec manual keys/policies. > [root@hp]~# kld > kldconfig kldload kldstat kldunload kldxref > [root@hp]~# kldunload ioat > [root@hp]~# kldunload aesni > [root@hp]~# kldunload ipsec > [root@hp]~# kldload aesni > [root@hp]~# kldload ipsec > [root@hp]~# service ipsec start > Installing ipsec manual keys/policies. > [root@hp]~# kldstat > Id Refs AddressSize Name > 1 11 0x8020 1ee58b0 kernel > 21 0x82319000 34c8 fdescfs.ko > 51 0x82337000 8c98 ioat.ko > 61 0x8231d000 a240 aesni.ko > 71 0x82328000 e350 ipsec.ko > > And after that the IPSec bench results are back to their previous value :-) > So rcorder needs to load aesni before ipsec. I don't think the issue is with rcorder though. I think the reason the ordering matters warrants further investigation. Is aesni not getting used when ipsec is loaded first? You can use dtrace with the script at https://github.com/bsdjhb/kdbg/blob/master/dtrace/crypto_drivers.d to see which driver is being used. Hmm, the crypto driver gets selected when keys for SAs are set, so if /etc/rc.d/ipsec is configuring SAs, then having it run before aesni is loaded would indeed cause this. However, that still isn't an issue with this commit, per se, it just exposed the lack of an explicit ordering requirement of /etc/rc.d/ipsec after loading aesni. The problem though is that kld_list is pretty generic, so it's hard to know when /etc/rc.d/kld should run. Possibly it should even run multiple times where subsequent runs try to load any modules not loaded by the previous runs? I suspect btw that you could just do 'sh /etc/rc.d/ipsec restart' post-boot without unloading any modules which would also fix your benchmark. I think long term we want OCF's notions of sessions to be a bit more fluid such that "client" sessions for things like GELI and IPSec can be backed by one or more "driver" sessions (including "driver" sessions coming and going as devices come and go). That's a fair bit more work however. -- John Baldwin ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r365449 - head/sbin/rcorder
On Tue, Sep 8, 2020 at 12:36 PM Andrey V. Elsukov wrote: > Author: ae > Date: Tue Sep 8 10:36:11 2020 > New Revision: 365449 > URL: https://svnweb.freebsd.org/changeset/base/365449 > > Log: > Add a few features to rcorder: > > > Hi Andrey, I've spent some time bisecting an IPSec gateway performance regression on -head that points to this commit. So my bench uses a simple static IPSec aes-gcm-16 setup, and their results vary a lot between those 2 revisions: - r365448: Estimated Equilibrium Ethernet throughput= 1413 Mb/s (maximum value seen: 1419 Mb/s) - r365449: Estimated Equilibrium Ethernet throughput= 469 Mb/s (maximum value seen: 469 Mb/s) How could a modification to the rcoder be the source cause of that ? My rc.conf contains this: kld_list="aesni" ipsec_enable="YES" So My first thought was that the aesni module wasn't loaded anymore: Before upgrade: [root@hp]~# uname -a FreeBSD hp 13.0-CURRENT FreeBSD 13.0-CURRENT #15 r365448M: Thu Sep 17 18:17:58 CEST 2020 oliv...@lame4.bsdrp.net:/usr/src/amd64.amd64/sys/amd64 amd64 [root@hp]~# kldstat Id Refs AddressSize Name 1 11 0x8020 1ee58b0 kernel 21 0x82319000 34c8 fdescfs.ko 31 0x8231d000 a240 aesni.ko 41 0x82328000 8c98 ioat.ko 51 0x82331000 e350 ipsec.ko Then after upgrade: [root@hp]~# uname -a FreeBSD hp 13.0-CURRENT FreeBSD 13.0-CURRENT #14 r365449M: Thu Sep 17 17:01:35 CEST 2020 oliv...@lame4.bsdrp.net:/usr/src/amd64.amd64/sys/amd64 amd64 [root@hp]~# kldstat Id Refs AddressSize Name 1 11 0x8020 1ee58b0 kernel 21 0x82319000 34c8 fdescfs.ko 31 0x8231d000 e350 ipsec.ko 41 0x8232c000 a240 aesni.ko 51 0x82337000 8c98 ioat.ko => aesni.ko is correctly loaded, so it is not the problem, but notice the order of the kernel modules that have changed. Could be this the source of the problem ? Let's try: [root@hp]~# service ipsec stop Clearing ipsec manual keys/policies. [root@hp]~# kld kldconfig kldload kldstat kldunload kldxref [root@hp]~# kldunload ioat [root@hp]~# kldunload aesni [root@hp]~# kldunload ipsec [root@hp]~# kldload aesni [root@hp]~# kldload ipsec [root@hp]~# service ipsec start Installing ipsec manual keys/policies. [root@hp]~# kldstat Id Refs AddressSize Name 1 11 0x8020 1ee58b0 kernel 21 0x82319000 34c8 fdescfs.ko 51 0x82337000 8c98 ioat.ko 61 0x8231d000 a240 aesni.ko 71 0x82328000 e350 ipsec.ko And after that the IPSec bench results are back to their previous value :-) So rcorder needs to load aesni before ipsec. Regards, Olivier ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r365449 - head/sbin/rcorder
Hi,
I think this change warrants an entry in RELNOTES.
Cheers,
Mitchell
On Tue, Sep 8, 2020 at 7:36 AM Andrey V. Elsukov wrote:
>
> Author: ae
> Date: Tue Sep 8 10:36:11 2020
> New Revision: 365449
> URL: https://svnweb.freebsd.org/changeset/base/365449
>
> Log:
> Add a few features to rcorder:
>
> o Enhance dependency loop logging: print full chain instead of the
> last link competing the loop;
> o Add -g option to generate dependency graph suitable for GraphViz
> visualization, loops and other graph generation issues are highlighted
> automatically;
> o Add -p option that enables grouping items that can be processed in
> parallel.
>
> Submitted by: Boris Lytochkin
> Reviewed by: melifaro
> MFC after:1 week
> Differential Revision:https://reviews.freebsd.org/D25389
>
> Modified:
> head/sbin/rcorder/rcorder.8
> head/sbin/rcorder/rcorder.c
>
> Modified: head/sbin/rcorder/rcorder.8
> ==
> --- head/sbin/rcorder/rcorder.8 Tue Sep 8 07:37:45 2020(r365448)
> +++ head/sbin/rcorder/rcorder.8 Tue Sep 8 10:36:11 2020(r365449)
> @@ -31,7 +31,7 @@
> .\"
> .\" $FreeBSD$
> .\"
> -.Dd June 22, 2020
> +.Dd September 8, 2020
> .Dt RCORDER 8
> .Os
> .Sh NAME
> @@ -39,6 +39,7 @@
> .Nd print a dependency ordering of interdependent files
> .Sh SYNOPSIS
> .Nm
> +.Op Fl gp
> .Op Fl k Ar keep
> .Op Fl s Ar skip
> .Ar
> @@ -95,6 +96,9 @@ is reached, parsing stops.
> .Pp
> The options are as follows:
> .Bl -tag -width "-k keep"
> +.It Fl g
> +Produce a GraphViz (.dot) of the complete dependency graph instead of
> +plaintext calling order list.
> .It Fl k Ar keep
> Add the specified keyword to the
> .Dq "keep list" .
> @@ -102,6 +106,9 @@ If any
> .Fl k
> option is given, only those files containing the matching keyword are listed.
> This option can be specified multiple times.
> +.It Fl p
> +Generate ordering suitable for parallel startup, placing files that can be
> +executed simultaneously on the same line.
> .It Fl s Ar skip
> Add the specified keyword to the
> .Dq "skip list" .
> @@ -178,19 +185,46 @@ The
> utility may print one of the following error messages and exit with a
> non-zero
> status if it encounters an error while processing the file list.
> .Bl -diag
> -.It "Requirement %s has no providers, aborting."
> +.It "Requirement %s in file %s has no providers."
> No file has a
> .Ql PROVIDE
> line corresponding to a condition present in a
> .Ql REQUIRE
> line in another file.
> -.It "Circular dependency on provision %s, aborting."
> +.It "Circular dependency on provision %s in file %s."
> A set of files has a circular dependency which was detected while
> processing the stated condition.
> -.It "Circular dependency on file %s, aborting."
> +Loop visualization follows this message.
> +.It "Circular dependency on file %s."
> A set of files has a circular dependency which was detected while
> processing the stated file.
> +.It "%s was seen in circular dependencies for %d times."
> +Each node that was a part of circular dependency loops reports total number
> of
> +such encounters.
> +Start with files having biggest counter when fighting with broken
> dependencies.
> .El
> +.Sh DIAGNOSTICS WITH GRAPHVIZ
> +Direct dependency is drawn with solid line,
> +.Ql BEFORE
> +dependency is drawn as a dashed line.
> +Each node of a graph represents an item from
> +.Ql PROVIDE
> +lines.
> +In case there are more than one file providing an item, a list of filenames
> +shortened with
> +.Xr basename 3
> +is shown.
> +Shortened filenames are also shown in case
> +.Ql PROVIDE
> +item does not match file name.
> +.Pp
> +Edges and nodes where circular dependencies were detected are drawn bold red.
> +If a file has an item in
> +.Ql REQUIRE
> +or in
> +.Ql BEFORE
> +that could not be provided,
> +this missing provider and the requirement will be drawn bold red as well.
> .Sh SEE ALSO
> .Xr acpiconf 8 ,
> .Xr rc 8 ,
>
> Modified: head/sbin/rcorder/rcorder.c
> ==
> --- head/sbin/rcorder/rcorder.c Tue Sep 8 07:37:45 2020(r365448)
> +++ head/sbin/rcorder/rcorder.c Tue Sep 8 10:36:11 2020(r365449)
> @@ -9,6 +9,8 @@
> * All rights reserved.
> * Copyright (c) 1998
> * Perry E. Metzger. All rights reserved.
> + * Copyright (c) 2020
> + * Boris N. Lytochkin. All rights reserved.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions
> @@ -48,6 +50,8 @@ __FBSDID("$FreeBSD$");
> #include
> #include
> #include
> +#include
> +#include
>
> #include "ealloc.h"
> #include "sprite.h"
> @@ -75,17 +79,21 @@ static int debug = 0;
> #define KEYWORDS_STR "# KEYWORDS:"
> #define KEYWORDS_LEN (sizeof(KEYWORDS_STR) - 1)
>
> +#defineFAKE_PROV_NAME "fake_
svn commit: r365449 - head/sbin/rcorder
Author: ae
Date: Tue Sep 8 10:36:11 2020
New Revision: 365449
URL: https://svnweb.freebsd.org/changeset/base/365449
Log:
Add a few features to rcorder:
o Enhance dependency loop logging: print full chain instead of the
last link competing the loop;
o Add -g option to generate dependency graph suitable for GraphViz
visualization, loops and other graph generation issues are highlighted
automatically;
o Add -p option that enables grouping items that can be processed in
parallel.
Submitted by: Boris Lytochkin
Reviewed by: melifaro
MFC after:1 week
Differential Revision:https://reviews.freebsd.org/D25389
Modified:
head/sbin/rcorder/rcorder.8
head/sbin/rcorder/rcorder.c
Modified: head/sbin/rcorder/rcorder.8
==
--- head/sbin/rcorder/rcorder.8 Tue Sep 8 07:37:45 2020(r365448)
+++ head/sbin/rcorder/rcorder.8 Tue Sep 8 10:36:11 2020(r365449)
@@ -31,7 +31,7 @@
.\"
.\" $FreeBSD$
.\"
-.Dd June 22, 2020
+.Dd September 8, 2020
.Dt RCORDER 8
.Os
.Sh NAME
@@ -39,6 +39,7 @@
.Nd print a dependency ordering of interdependent files
.Sh SYNOPSIS
.Nm
+.Op Fl gp
.Op Fl k Ar keep
.Op Fl s Ar skip
.Ar
@@ -95,6 +96,9 @@ is reached, parsing stops.
.Pp
The options are as follows:
.Bl -tag -width "-k keep"
+.It Fl g
+Produce a GraphViz (.dot) of the complete dependency graph instead of
+plaintext calling order list.
.It Fl k Ar keep
Add the specified keyword to the
.Dq "keep list" .
@@ -102,6 +106,9 @@ If any
.Fl k
option is given, only those files containing the matching keyword are listed.
This option can be specified multiple times.
+.It Fl p
+Generate ordering suitable for parallel startup, placing files that can be
+executed simultaneously on the same line.
.It Fl s Ar skip
Add the specified keyword to the
.Dq "skip list" .
@@ -178,19 +185,46 @@ The
utility may print one of the following error messages and exit with a non-zero
status if it encounters an error while processing the file list.
.Bl -diag
-.It "Requirement %s has no providers, aborting."
+.It "Requirement %s in file %s has no providers."
No file has a
.Ql PROVIDE
line corresponding to a condition present in a
.Ql REQUIRE
line in another file.
-.It "Circular dependency on provision %s, aborting."
+.It "Circular dependency on provision %s in file %s."
A set of files has a circular dependency which was detected while
processing the stated condition.
-.It "Circular dependency on file %s, aborting."
+Loop visualization follows this message.
+.It "Circular dependency on file %s."
A set of files has a circular dependency which was detected while
processing the stated file.
+.It "%s was seen in circular dependencies for %d times."
+Each node that was a part of circular dependency loops reports total number of
+such encounters.
+Start with files having biggest counter when fighting with broken dependencies.
.El
+.Sh DIAGNOSTICS WITH GRAPHVIZ
+Direct dependency is drawn with solid line,
+.Ql BEFORE
+dependency is drawn as a dashed line.
+Each node of a graph represents an item from
+.Ql PROVIDE
+lines.
+In case there are more than one file providing an item, a list of filenames
+shortened with
+.Xr basename 3
+is shown.
+Shortened filenames are also shown in case
+.Ql PROVIDE
+item does not match file name.
+.Pp
+Edges and nodes where circular dependencies were detected are drawn bold red.
+If a file has an item in
+.Ql REQUIRE
+or in
+.Ql BEFORE
+that could not be provided,
+this missing provider and the requirement will be drawn bold red as well.
.Sh SEE ALSO
.Xr acpiconf 8 ,
.Xr rc 8 ,
Modified: head/sbin/rcorder/rcorder.c
==
--- head/sbin/rcorder/rcorder.c Tue Sep 8 07:37:45 2020(r365448)
+++ head/sbin/rcorder/rcorder.c Tue Sep 8 10:36:11 2020(r365449)
@@ -9,6 +9,8 @@
* All rights reserved.
* Copyright (c) 1998
* Perry E. Metzger. All rights reserved.
+ * Copyright (c) 2020
+ * Boris N. Lytochkin. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -48,6 +50,8 @@ __FBSDID("$FreeBSD$");
#include
#include
#include
+#include
+#include
#include "ealloc.h"
#include "sprite.h"
@@ -75,17 +79,21 @@ static int debug = 0;
#define KEYWORDS_STR "# KEYWORDS:"
#define KEYWORDS_LEN (sizeof(KEYWORDS_STR) - 1)
+#defineFAKE_PROV_NAME "fake_prov_"
+
static int exit_code;
static int file_count;
static char **file_list;
-typedef int bool;
#define TRUE 1
#define FALSE 0
typedef bool flag;
#define SET TRUE
#define RESET FALSE
+static flag do_graphviz = false;
+static flag do_parallel = false;
+
static Hash_Table provide_hash_s, *provide_hash;
typedef struct provnode provnode;
@@ -97,12 +105,14 @@ typedef struct strnodelist strnodelist;
st
