Re: tls_load_file.3: tls_config_set_*_file() load files into memory
On Tue, Jun 22, 2021 at 04:48:39AM +0200, Theo Buehler wrote: > > > > Feedback? OK? > > You have two overlong lines as indicated below. I would have thought > that mandoc -Tlint complains about that, but apparently it doesn't have > such a warning... With those wrapped, > yes, there is no feedback on long lines. although we try to keep the source less than 80 width, there are some places where it is not possible. i'm not sure whether adding a warning would be helpful or disruptive. jmc
Re: tls_load_file.3: tls_config_set_*_file() load files into memory
On Mon, Jun 21, 2021 at 11:26:41PM +, Klemens Nanni wrote: > > Thanks. tls_config_add_*_file also load files into memory, but given > this patch I think their usage of "add" in the manual is enough to infer > that files will also be loaded and added, so no need to change those as > well, I think. > > This should be the complete diff. > > Feedback? OK? > > > Index: man/tls_load_file.3 > === > RCS file: /cvs/src/lib/libtls/man/tls_load_file.3,v > retrieving revision 1.11 > diff -u -p -r1.11 tls_load_file.3 > --- man/tls_load_file.3 29 Nov 2018 14:24:23 - 1.11 > +++ man/tls_load_file.3 21 Jun 2021 23:24:58 - > @@ -217,8 +217,7 @@ call, ensuring that the memory contents > returns the path of the file that contains the default root certificates. > .Pp > .Fn tls_config_set_ca_file > -sets the filename used to load a file > -containing the root certificates. > +loads a file containing the root certificates. > .Pp > .Fn tls_config_set_ca_path > sets the path (directory) which should be searched for root > @@ -228,41 +227,39 @@ certificates. > sets the root certificates directly from memory. > .Pp > .Fn tls_config_set_cert_file > -sets file from which the public certificate will be read. > +loads a file containing the public certificate. > .Pp > .Fn tls_config_set_cert_mem > sets the public certificate directly from memory. > .Pp > .Fn tls_config_set_crl_file > -sets the filename used to load a file containing the > -Certificate Revocation List (CRL). > +loads a file containing the Certificate Revocation List (CRL). > .Pp > .Fn tls_config_set_crl_mem > sets the CRL directly from memory. > .Pp > .Fn tls_config_set_key_file > -sets the file from which the private key will be read. > +loads a file containing the private key. > .Pp > .Fn tls_config_set_key_mem > directly sets the private key from memory. > .Pp > .Fn tls_config_set_ocsp_staple_file > -sets a DER-encoded OCSP response to be stapled during the TLS handshake from > -the specified file. > +loads a file containing a DER-encoded OCSP response to be stapled during the > TLS handshake. > .Pp > .Fn tls_config_set_ocsp_staple_mem > sets a DER-encoded OCSP response to be stapled during the TLS handshake from > memory. > .Pp > .Fn tls_config_set_keypair_file > -sets the files from which the public certificate, and private key will be > read. > +loads two files from which the public certificate, and private key will be > read. this is a weird place for a comma. i would remove it. jmc > .Pp > .Fn tls_config_set_keypair_mem > directly sets the public certificate, and private key from memory. > .Pp > .Fn tls_config_set_keypair_ocsp_file > -sets the files from which the public certificate, private key, and > DER-encoded > -OCSP staple will be read. > +loads three files containing the public certificate, private key, and > DER-encoded > +OCSP staple. > .Pp > .Fn tls_config_set_keypair_ocsp_mem > directly sets the public certificate, private key, and DER-encoded OCSP > staple >
Re: tls_load_file.3: tls_config_set_*_file() load files into memory
On Mon, Jun 21, 2021 at 11:26:41PM +, Klemens Nanni wrote: > On Sun, Jun 20, 2021 at 09:32:36PM +0200, Theo Buehler wrote: > > On Sat, Jun 19, 2021 at 03:34:39PM +, Klemens Nanni wrote: > > > On Thu, Jun 10, 2021 at 10:26:15PM +, Klemens Nanni wrote: > > > > tls_config_set_ca_file(3) and tls_config_set_cert_file(3) do not just > > > > set the file paths (like tls_config_set_ca_path(3) does), they do load > > > > the given file into memory directly using tls_config_load_file(). > > > > > > > > This distinction is important because it means a later tls_connect(3) > > > > will not do any file I/O (at least regarding those files), which is > > > > relevant when for example pleding without "[rwc]path" after loading > > > > files into memory and before doing tls_connect(3). > > > > > > > > The manual's current wording made me use the following due to above way > > > > of pleding a program: > > > > > > > > tls_load_file() > > > > tls_config_set_ca_mem() > > > > tls_unload_file() > > > > > > > > While in fact the following does the same (in my case): > > > > > > > > tls_config_set_ca_file() > > > > > > > > > > > > So clarify this in the manual. > > > > > > > > Feedback? Objections? OK? > > > > > > Ping. > > > > You're right. This was changed in tls_config.c r1.26 (Aug 2016) and the > > documentation wasn't updated. However, the diff is incomplete as this > > concerns all tls_config_set_*_file functions: > > > > tls_config_set_ca_file > > tls_config_set_cert_file > > tls_config_set_crl_file > > tls_config_set_key_file > > tls_config_set_keypair_file > > tls_config_set_keypair_ocsp_file > > tls_config_set_ocsp_staple_file > > Thanks. tls_config_add_*_file also load files into memory, but given > this patch I think their usage of "add" in the manual is enough to infer > that files will also be loaded and added, so no need to change those as > well, I think. Agreed. > This should be the complete diff. > > Feedback? OK? You have two overlong lines as indicated below. I would have thought that mandoc -Tlint complains about that, but apparently it doesn't have such a warning... With those wrapped, ok tb > > Index: man/tls_load_file.3 > === > RCS file: /cvs/src/lib/libtls/man/tls_load_file.3,v > retrieving revision 1.11 > diff -u -p -r1.11 tls_load_file.3 > --- man/tls_load_file.3 29 Nov 2018 14:24:23 - 1.11 > +++ man/tls_load_file.3 21 Jun 2021 23:24:58 - > @@ -217,8 +217,7 @@ call, ensuring that the memory contents > returns the path of the file that contains the default root certificates. > .Pp > .Fn tls_config_set_ca_file > -sets the filename used to load a file > -containing the root certificates. > +loads a file containing the root certificates. > .Pp > .Fn tls_config_set_ca_path > sets the path (directory) which should be searched for root > @@ -228,41 +227,39 @@ certificates. > sets the root certificates directly from memory. > .Pp > .Fn tls_config_set_cert_file > -sets file from which the public certificate will be read. > +loads a file containing the public certificate. > .Pp > .Fn tls_config_set_cert_mem > sets the public certificate directly from memory. > .Pp > .Fn tls_config_set_crl_file > -sets the filename used to load a file containing the > -Certificate Revocation List (CRL). > +loads a file containing the Certificate Revocation List (CRL). > .Pp > .Fn tls_config_set_crl_mem > sets the CRL directly from memory. > .Pp > .Fn tls_config_set_key_file > -sets the file from which the private key will be read. > +loads a file containing the private key. > .Pp > .Fn tls_config_set_key_mem > directly sets the private key from memory. > .Pp > .Fn tls_config_set_ocsp_staple_file > -sets a DER-encoded OCSP response to be stapled during the TLS handshake from > -the specified file. > +loads a file containing a DER-encoded OCSP response to be stapled during the > TLS handshake. line break before "during" > .Pp > .Fn tls_config_set_ocsp_staple_mem > sets a DER-encoded OCSP response to be stapled during the TLS handshake from > memory. > .Pp > .Fn tls_config_set_keypair_file > -sets the files from which the public certificate, and private key will be > read. > +loads two files from which the public certificate, and private key will be > read. > .Pp > .Fn tls_config_set_keypair_mem > directly sets the public certificate, and private key from memory. > .Pp > .Fn tls_config_set_keypair_ocsp_file > -sets the files from which the public certificate, private key, and > DER-encoded > -OCSP staple will be read. > +loads three files containing the public certificate, private key, and > DER-encoded line break: ... key, and DER-encoded OCSP staple. > +OCSP staple. > .Pp > .Fn tls_config_set_keypair_ocsp_mem > directly sets the public certificate, private key, and DER-encoded OCSP > staple
Re: [External] : Re: parallel forwarding vs. bridges
On Wed, Jun 16, 2021 at 02:59:19AM +0200, Alexandr Nedvedicky wrote:
> Hello,
>
>
> >
> > as above, copyout with a sleeping lock is fine.
> >
> > the whole point of my change is to give us the ability to lock in the
> > forwarding path separately to locking in the ioctl path. half of that is
> > so we can copyout safely. the other half is to avoid letting the ioctl
> > path block packet processing if we can avoid it as an alternative to
> > having the network stack having to yield the cpu.
>
> I see. my confusion came from fact I've forgot pflock got turned to mutex,
> when we saw the crash.
>
> >
> > > let's take a look at this part of pf_purge_expired_states()
> > > from your diff:
> > >
> > > 1543 NET_LOCK();
> > > 1544 rw_enter_write(&pf_state_list.pfs_rwl);
> > > 1545 PF_LOCK();
> > > 1546 PF_STATE_ENTER_WRITE();
> > > 1547 SLIST_FOREACH(st, &gcl, gc_list) {
> > > 1548 if (st->timeout != PFTM_UNLINKED)
> > > 1549 pf_remove_state(st);
> > > 1550
> > > 1551 pf_free_state(st);
> > > 1552 }
> > > 1553 PF_STATE_EXIT_WRITE();
> > > 1554 PF_UNLOCK();
> > > 1555 rw_exit_write(&pf_state_list.pfs_rwl);
> > >
> > > at line 1543 we grab NET_LOCK(), at line 1544 we are trying
> > > to grab new lock (pf_state_list.pfs_rwl) exclusively.
> > >
> > > with your change we might be running into situation, where we do
> > > copyout() as a
> > > reader on pf_state_list.pfs_rwl. Then we grab NET_LOCK() and attempt to
> > > acquire
> > > pf_state_list.pfs_rwl exclusively, which is still occupied by guy, who
> > > might be
> > > doing uvm_fault() in copyout(9f).
> > >
> > > I'm just worried we may be trading one bug for another bug. may be my
> > > concern
> > > is just a false alarm here. I don't know.
> >
> > no, these things are all worth discussing.
> >
> > it's definitely possible there's bugs in here, but im pretty confident
> > it's not the copyout one.
> >
>
> it seems to work. I'm running your diff with bluhm's parallel diff
> and do occasional pfctl -Fs/pfctl -ss under a load. so far so good.
>
>
>
> > > I guess 'pfgpurge_expired_fragment(s);' is unintentional change,
> > > right?
> >
> > yeah, i dont know how i did that. vi is hard?
>
> sure it is...
> thanks for fixing the nits.
>
>
>
> > Index: if_pfsync.c
> > ===
> > RCS file: /cvs/src/sys/net/if_pfsync.c,v
> > retrieving revision 1.292
> > diff -u -p -r1.292 if_pfsync.c
> > --- if_pfsync.c 15 Jun 2021 10:10:22 - 1.292
> > +++ if_pfsync.c 15 Jun 2021 11:21:20 -
> > @@ -2545,22 +2545,34 @@ pfsync_bulk_start(void)
> > {
> > struct pfsync_softc *sc = pfsyncif;
>
> have not spot anything suspicious in if_pfsync.c
>
> the new diff reads fine to me.
>
> OK sashan
I've been running versions of this diff in production at work, and have
hit a few panics and asserts. All the issues we've hit should be
addressed in this diff.
The first issue was that pfsync could be in the processing of sending a
deferred packet while it's being removed from the state tree. Part of
that removal process is stripping the state keys from the state, which
pfsync uses to determine the address family so it knows which ip output
routine to use. The quick and dirty fix to this is to have pfsync check
if timeout state to see if the state is unlinked or not. This currently
relies on pfsync undefer and pf being serialised by the NET_LOCK.
The second is that the timeout member on a state can change while the
purge task is looking at it. We hit this assert in pf_state_expires:
KASSERT(state->timeout != PFTM_UNLINKED);
pf_state_expires was called from the purge code like this:
if ((cur->timeout == PFTM_UNLINKED) ||
(pf_state_expires(cur) <= getuptime()))
SLIST_INSERT_HEAD(&gcl, cur, gc_list);
With my new locking scheme here, the state purge code is called without
any of the locks that would serialise access the state->timeout
variable. I think I found a solution to this without having to
reintroduce extra locking, which should allow us to keep the purge scan
running concurrently with pf actually handling packets.
Index: pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1118
diff -u -p -r1.1118 pf.c
--- pf.c1 Jun 2021 09:57:11 - 1.1118
+++ pf.c22 Jun 2021 01:34:36 -
@@ -259,6 +259,7 @@ void pf_state_key_link_inpcb(struct
p
voidpf_state_key_unlink_inpcb(struct pf_state_key *);
voidpf_inpcb_unlink_state_key(struct inpcb *);
voidpf_pktenqueue_delayed(void *);
+int32_t pf_state_expires(const struct pf_state *,
uint8_t);
#if NPFLOG >
Re: tls_load_file.3: tls_config_set_*_file() load files into memory
On Sun, Jun 20, 2021 at 09:32:36PM +0200, Theo Buehler wrote: > On Sat, Jun 19, 2021 at 03:34:39PM +, Klemens Nanni wrote: > > On Thu, Jun 10, 2021 at 10:26:15PM +, Klemens Nanni wrote: > > > tls_config_set_ca_file(3) and tls_config_set_cert_file(3) do not just > > > set the file paths (like tls_config_set_ca_path(3) does), they do load > > > the given file into memory directly using tls_config_load_file(). > > > > > > This distinction is important because it means a later tls_connect(3) > > > will not do any file I/O (at least regarding those files), which is > > > relevant when for example pleding without "[rwc]path" after loading > > > files into memory and before doing tls_connect(3). > > > > > > The manual's current wording made me use the following due to above way > > > of pleding a program: > > > > > > tls_load_file() > > > tls_config_set_ca_mem() > > > tls_unload_file() > > > > > > While in fact the following does the same (in my case): > > > > > > tls_config_set_ca_file() > > > > > > > > > So clarify this in the manual. > > > > > > Feedback? Objections? OK? > > > > Ping. > > You're right. This was changed in tls_config.c r1.26 (Aug 2016) and the > documentation wasn't updated. However, the diff is incomplete as this > concerns all tls_config_set_*_file functions: > > tls_config_set_ca_file > tls_config_set_cert_file > tls_config_set_crl_file > tls_config_set_key_file > tls_config_set_keypair_file > tls_config_set_keypair_ocsp_file > tls_config_set_ocsp_staple_file Thanks. tls_config_add_*_file also load files into memory, but given this patch I think their usage of "add" in the manual is enough to infer that files will also be loaded and added, so no need to change those as well, I think. This should be the complete diff. Feedback? OK? Index: man/tls_load_file.3 === RCS file: /cvs/src/lib/libtls/man/tls_load_file.3,v retrieving revision 1.11 diff -u -p -r1.11 tls_load_file.3 --- man/tls_load_file.3 29 Nov 2018 14:24:23 - 1.11 +++ man/tls_load_file.3 21 Jun 2021 23:24:58 - @@ -217,8 +217,7 @@ call, ensuring that the memory contents returns the path of the file that contains the default root certificates. .Pp .Fn tls_config_set_ca_file -sets the filename used to load a file -containing the root certificates. +loads a file containing the root certificates. .Pp .Fn tls_config_set_ca_path sets the path (directory) which should be searched for root @@ -228,41 +227,39 @@ certificates. sets the root certificates directly from memory. .Pp .Fn tls_config_set_cert_file -sets file from which the public certificate will be read. +loads a file containing the public certificate. .Pp .Fn tls_config_set_cert_mem sets the public certificate directly from memory. .Pp .Fn tls_config_set_crl_file -sets the filename used to load a file containing the -Certificate Revocation List (CRL). +loads a file containing the Certificate Revocation List (CRL). .Pp .Fn tls_config_set_crl_mem sets the CRL directly from memory. .Pp .Fn tls_config_set_key_file -sets the file from which the private key will be read. +loads a file containing the private key. .Pp .Fn tls_config_set_key_mem directly sets the private key from memory. .Pp .Fn tls_config_set_ocsp_staple_file -sets a DER-encoded OCSP response to be stapled during the TLS handshake from -the specified file. +loads a file containing a DER-encoded OCSP response to be stapled during the TLS handshake. .Pp .Fn tls_config_set_ocsp_staple_mem sets a DER-encoded OCSP response to be stapled during the TLS handshake from memory. .Pp .Fn tls_config_set_keypair_file -sets the files from which the public certificate, and private key will be read. +loads two files from which the public certificate, and private key will be read. .Pp .Fn tls_config_set_keypair_mem directly sets the public certificate, and private key from memory. .Pp .Fn tls_config_set_keypair_ocsp_file -sets the files from which the public certificate, private key, and DER-encoded -OCSP staple will be read. +loads three files containing the public certificate, private key, and DER-encoded +OCSP staple. .Pp .Fn tls_config_set_keypair_ocsp_mem directly sets the public certificate, private key, and DER-encoded OCSP staple
crypto kernel lock
On Thu, Jun 17, 2021 at 03:19:11PM +0200, Alexander Bluhm wrote:
> On Thu, Jun 17, 2021 at 10:09:47AM +0200, Martin Pieuchot wrote:
> > Could you annotate which field is being protected by the KERNEL_LOCK()?
>
> No. I do not want to invest into fine grained crypto locking. I
> need a stable test machine.
Now my machine is stable again, I can do some annotations.
- remove unused variable cryptodesc_pool
- document global variables in crypto.c
- assert kernel lock where needed
- remove dead code from crypto_get_driverid()
- move crypto_init() prototype into header file
ok?
bluhm
Index: crypto/crypto.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/crypto/crypto.c,v
retrieving revision 1.82
diff -u -p -r1.82 crypto.c
--- crypto/crypto.c 30 Mar 2020 17:48:39 - 1.82
+++ crypto/crypto.c 21 Jun 2021 21:00:19 -
@@ -27,16 +27,23 @@
#include
-void crypto_init(void);
-
-struct cryptocap *crypto_drivers = NULL;
-int crypto_drivers_num = 0;
-
-struct pool cryptop_pool;
-struct pool cryptodesc_pool;
+/*
+ * Locks used to protect struct members in this file:
+ * A allocated during driver attach, no hotplug, no detach
+ * I initialized by main()
+ * K modified with kernel lock
+ */
-struct taskq *crypto_taskq;
-struct taskq *crypto_taskq_mpsafe;
+struct cryptocap *crypto_drivers; /* [A] array allocated by driver
+ [K] driver data and session count */
+int crypto_drivers_num = 0;/* [A] attached drivers array size */
+
+struct pool cryptop_pool; /* [I] set of crypto descriptors */
+
+struct taskq *crypto_taskq;/* [I] run crypto_invoke() and callback
+ with kernel lock */
+struct taskq *crypto_taskq_mpsafe; /* [I] run crypto_invoke()
+ without kernel lock */
/*
* Create a new session.
@@ -52,6 +59,8 @@ crypto_newsession(u_int64_t *sid, struct
if (crypto_drivers == NULL)
return EINVAL;
+ KERNEL_ASSERT_LOCKED();
+
s = splvm();
/*
@@ -186,6 +195,8 @@ crypto_freesession(u_int64_t sid)
if (hid >= crypto_drivers_num)
return ENOENT;
+ KERNEL_ASSERT_LOCKED();
+
s = splvm();
if (crypto_drivers[hid].cc_sessions)
@@ -215,6 +226,9 @@ crypto_get_driverid(u_int8_t flags)
{
struct cryptocap *newdrv;
int i, s;
+
+ /* called from attach routines */
+ KERNEL_ASSERT_LOCKED();
s = splvm();
@@ -241,39 +255,33 @@ crypto_get_driverid(u_int8_t flags)
}
/* Out of entries, allocate some more. */
- if (i == crypto_drivers_num) {
- if (crypto_drivers_num >= CRYPTO_DRIVERS_MAX) {
- splx(s);
- return -1;
- }
-
- newdrv = mallocarray(crypto_drivers_num,
- 2 * sizeof(struct cryptocap), M_CRYPTO_DATA, M_NOWAIT);
- if (newdrv == NULL) {
- splx(s);
- return -1;
- }
+ if (crypto_drivers_num >= CRYPTO_DRIVERS_MAX) {
+ splx(s);
+ return -1;
+ }
- memcpy(newdrv, crypto_drivers,
- crypto_drivers_num * sizeof(struct cryptocap));
- bzero(&newdrv[crypto_drivers_num],
- crypto_drivers_num * sizeof(struct cryptocap));
+ newdrv = mallocarray(crypto_drivers_num,
+ 2 * sizeof(struct cryptocap), M_CRYPTO_DATA, M_NOWAIT);
+ if (newdrv == NULL) {
+ splx(s);
+ return -1;
+ }
- newdrv[i].cc_sessions = 1; /* Mark */
- newdrv[i].cc_flags = flags;
+ memcpy(newdrv, crypto_drivers,
+ crypto_drivers_num * sizeof(struct cryptocap));
+ bzero(&newdrv[crypto_drivers_num],
+ crypto_drivers_num * sizeof(struct cryptocap));
- free(crypto_drivers, M_CRYPTO_DATA,
- crypto_drivers_num * sizeof(struct cryptocap));
+ newdrv[i].cc_sessions = 1; /* Mark */
+ newdrv[i].cc_flags = flags;
- crypto_drivers_num *= 2;
- crypto_drivers = newdrv;
- splx(s);
- return i;
- }
+ free(crypto_drivers, M_CRYPTO_DATA,
+ crypto_drivers_num * sizeof(struct cryptocap));
- /* Shouldn't really get here... */
+ crypto_drivers_num *= 2;
+ crypto_drivers = newdrv;
splx(s);
- return -1;
+ return i;
}
/*
@@ -287,11 +295,13 @@ crypto_register(u_int32_t driverid, int
{
int s, i;
-
if (driverid >= crypto_drivers_num || alg == NULL ||
crypto_drivers == NULL)
return EINVAL;
+ /* called from attach routines */
+ KERNEL_ASSERT
UGREEN USB to RS-232 corrected patch
Index: uplcom.c
===
RCS file: /cvs/src/sys/dev/usb/uplcom.c,v
retrieving revision 1.77
diff -u -p -u -p -r1.77 uplcom.c
--- uplcom.c27 Jan 2021 17:28:19 - 1.77
+++ uplcom.c19 Jun 2021 19:25:50 -
@@ -162,6 +162,7 @@ static const struct usb_devno uplcom_dev
{ USB_VENDOR_PROLIFIC, USB_PRODUCT_PROLIFIC_PL2303GC },
{ USB_VENDOR_PROLIFIC, USB_PRODUCT_PROLIFIC_PL2303X },
{ USB_VENDOR_PROLIFIC, USB_PRODUCT_PROLIFIC_PL2303X2 },
+ { USB_VENDOR_PROLIFIC, USB_PRODUCT_PROLIFIC_PL2303ALB },
{ USB_VENDOR_PROLIFIC, USB_PRODUCT_PROLIFIC_RSAQ2 },
{ USB_VENDOR_PROLIFIC, USB_PRODUCT_PROLIFIC_PL2303BENQ },
{ USB_VENDOR_PROLIFIC2, USB_PRODUCT_PROLIFIC2_PL2303 },
Index: usbdevs
===
RCS file: /cvs/src/sys/dev/usb/usbdevs,v
retrieving revision 1.740
diff -u -p -u -p -r1.740 usbdevs
--- usbdevs 18 May 2021 14:23:03 - 1.740
+++ usbdevs 19 Jun 2021 19:25:50 -
@@ -3649,6 +3649,7 @@ product PROLIFIC PL2303GC 0x23a3 PL2303
product PROLIFIC PL25010x2501 PL2501 Host-Host
product PROLIFIC PL2303X 0xaaa0 PL2303 Serial
product PROLIFIC PL2303X2 0xaaa2 PL2303 Serial
+product PROLIFIC PL2303ALB 0x23c3 PL2303 Serial
/* Prolific(2) products */
product PROLIFIC2 PL2303 0x2303 PL2303 Serial
WIP iwx(4) Tx aggregation
This patch attempts to implement Tx aggregation support for iwx(4). It is not yet ready to be committed because of outstanding problems: - Under load the firmware throws a fatal firmware error every few minutes. - Starting a background scan under load can cause firmware errors and might error out when the driver attempts to flush Tx queues. However, roaming seems to be generally working while traffic is light. - Sometimes traffic seems to get stuck for no apparent reason and the driver won't recover without down/up. This is independent from the rx_reorder() fix which was committed today. Still, if you are craving up to 100 Mbit/s throughput right now, it won't hurt to give this patch a spin. Let me know about any issues you see. I just hope the list of known issues won't grow much longer :) AX201 users should be aware that I have only tested this on AX200 so far. Make sure to update to the latest if_iwx.c commit from today before applying this patch. diff refs/heads/master refs/heads/iwx-txagg blob - 21bc8be34500730264ad206513951a32fcb2d702 blob + f40a5d706cb73d745c1c4b7bcaf3681dfddfa518 --- sys/dev/pci/if_iwx.c +++ sys/dev/pci/if_iwx.c @@ -311,16 +311,14 @@ int iwx_ampdu_rx_start(struct ieee80211com *, struct i uint8_t); void iwx_ampdu_rx_stop(struct ieee80211com *, struct ieee80211_node *, uint8_t); +intiwx_ampdu_tx_start(struct ieee80211com *, struct ieee80211_node *, + uint8_t); void iwx_rx_ba_session_expired(void *); void iwx_reorder_timer_expired(void *); void iwx_sta_rx_agg(struct iwx_softc *, struct ieee80211_node *, uint8_t, uint16_t, uint16_t, int, int); -#ifdef notyet -intiwx_ampdu_tx_start(struct ieee80211com *, struct ieee80211_node *, +void iwx_sta_tx_agg_start(struct iwx_softc *, struct ieee80211_node *, uint8_t); -void iwx_ampdu_tx_stop(struct ieee80211com *, struct ieee80211_node *, - uint8_t); -#endif void iwx_ba_task(void *); intiwx_set_mac_addr_from_csr(struct iwx_softc *, struct iwx_nvm_data *); @@ -348,8 +346,11 @@ void iwx_rx_frame(struct iwx_softc *, struct mbuf *, i uint32_t, struct ieee80211_rxinfo *, struct mbuf_list *); void iwx_rx_tx_cmd_single(struct iwx_softc *, struct iwx_rx_packet *, struct iwx_node *); +void iwx_txd_done(struct iwx_softc *, struct iwx_tx_data *); +void iwx_txq_advance(struct iwx_softc *, struct iwx_tx_ring *, int); void iwx_rx_tx_cmd(struct iwx_softc *, struct iwx_rx_packet *, struct iwx_rx_data *); +void iwx_clear_oactive(struct iwx_softc *, struct iwx_tx_ring *); void iwx_rx_bmiss(struct iwx_softc *, struct iwx_rx_packet *, struct iwx_rx_data *); intiwx_binding_cmd(struct iwx_softc *, struct iwx_node *, uint32_t); @@ -369,8 +370,11 @@ void iwx_cmd_done(struct iwx_softc *, int, int, int); const struct iwx_rate *iwx_tx_fill_cmd(struct iwx_softc *, struct iwx_node *, struct ieee80211_frame *, struct iwx_tx_cmd_gen2 *); void iwx_tx_update_byte_tbl(struct iwx_tx_ring *, int, uint16_t, uint16_t); -intiwx_tx(struct iwx_softc *, struct mbuf *, struct ieee80211_node *, int); -intiwx_flush_tx_path(struct iwx_softc *); +intiwx_tx(struct iwx_softc *, struct mbuf *, struct ieee80211_node *); +intiwx_flush_sta_tids(struct iwx_softc *, int, uint16_t); +intiwx_wait_tx_queues_empty(struct iwx_softc *); +intiwx_drain_sta(struct iwx_softc *sc, struct iwx_node *, int); +intiwx_flush_sta(struct iwx_softc *, struct iwx_node *); intiwx_beacon_filter_send_cmd(struct iwx_softc *, struct iwx_beacon_filter_cmd *); intiwx_update_beacon_abort(struct iwx_softc *, struct iwx_node *, int); @@ -406,7 +410,7 @@ int iwx_scan_abort(struct iwx_softc *); intiwx_rs_rval2idx(uint8_t); uint16_t iwx_rs_ht_rates(struct iwx_softc *, struct ieee80211_node *, int); intiwx_rs_init(struct iwx_softc *, struct iwx_node *); -intiwx_enable_data_tx_queues(struct iwx_softc *); +intiwx_enable_mgmt_queue(struct iwx_softc *); intiwx_auth(struct iwx_softc *); intiwx_deauth(struct iwx_softc *); intiwx_assoc(struct iwx_softc *); @@ -422,6 +426,7 @@ voidiwx_delete_key(struct ieee80211com *, intiwx_media_change(struct ifnet *); void iwx_newstate_task(void *); intiwx_newstate(struct ieee80211com *, enum ieee80211_state, int); +void iwx_endbgscan_task(void *); void iwx_endscan(struct iwx_softc *); void iwx_fill_sf_command(struct iwx_softc *, struct iwx_sf_cfg_cmd *, struct ieee80211_node *); @@ -1692,20 +1697,21 @@ iwx_alloc_tx_ring(struct iwx_softc *sc, struct iwx_tx_ ring->desc = ring->desc_dma.vaddr; /* -* There is no need to allocate DMA buffers for unused rings. -* The hardware supports up to 31 Tx rings which is more +* The hardware supports up to 512 Tx rings which is more * than we currently need. * -*
Re: amd64: softintr_dispatch: remove kernel lock
On Thu, May 27, 2021 at 07:40:26PM -0500, Scott Cheloha wrote:
> On Sun, May 23, 2021 at 09:05:24AM +, Visa Hankala wrote:
> > When a CPU starts processing a soft interrupt, it reserves the handler
> > to prevent concurrent execution. If the soft interrupt gets rescheduled
> > during processing, the handler is run again by the same CPU. This breaks
> > FIFO ordering, though.
>
> If you want to preserve FIFO you can reinsert the handler at the queue
> tail. That would be more fair.
>
> If FIFO is the current behavior I think we ought to keep it.
I have updated the patch to preserve the FIFO order.
> > +STAILQ_HEAD(x86_soft_intr_queue, x86_soft_intrhand);
> > +
> > +struct x86_soft_intr_queue softintr_queue[X86_NSOFTINTR];
>
> Why did we switch to STAILQ? I know we don't have very many
> softintr_disestablish() calls but isn't O(1) removal worth the extra
> pointer?
I used STAILQ because it avoids the hassle of updating the list nodes'
back pointers. softintr_disestablish() with multiple items pending in
the queue is very rare in comparison to the normal softintr_schedule() /
softintr_dispatch() cycle.
However, I have changed the code back to using TAILQ.
Index: arch/amd64/amd64/softintr.c
===
RCS file: src/sys/arch/amd64/amd64/softintr.c,v
retrieving revision 1.10
diff -u -p -r1.10 softintr.c
--- arch/amd64/amd64/softintr.c 11 Sep 2020 09:27:09 - 1.10
+++ arch/amd64/amd64/softintr.c 21 Jun 2021 13:32:33 -
@@ -37,12 +37,33 @@
#include
#include
#include
+#include
+#include
#include
#include
-struct x86_soft_intr x86_soft_intrs[X86_NSOFTINTR];
+struct x86_soft_intrhand {
+ TAILQ_ENTRY(x86_soft_intrhand) sih_q;
+ void(*sih_fn)(void *);
+ void*sih_arg;
+ struct cpu_info *sih_runner;
+ int sih_which;
+ unsigned short sih_flags;
+ unsigned short sih_state;
+};
+
+#define SIF_MPSAFE 0x0001
+
+#define SIS_DYING 0x0001
+#define SIS_PENDING0x0002
+#define SIS_RESTART0x0004
+
+TAILQ_HEAD(x86_soft_intr_queue, x86_soft_intrhand);
+
+struct x86_soft_intr_queue softintr_queue[X86_NSOFTINTR];
+struct mutex softintr_lock = MUTEX_INITIALIZER(IPL_HIGH);
const int x86_soft_intr_to_ssir[X86_NSOFTINTR] = {
SIR_CLOCK,
@@ -58,15 +79,10 @@ const int x86_soft_intr_to_ssir[X86_NSOF
void
softintr_init(void)
{
- struct x86_soft_intr *si;
int i;
- for (i = 0; i < X86_NSOFTINTR; i++) {
- si = &x86_soft_intrs[i];
- TAILQ_INIT(&si->softintr_q);
- mtx_init(&si->softintr_lock, IPL_HIGH);
- si->softintr_ssir = x86_soft_intr_to_ssir[i];
- }
+ for (i = 0; i < nitems(softintr_queue); i++)
+ TAILQ_INIT(&softintr_queue[i]);
}
/*
@@ -78,31 +94,44 @@ void
softintr_dispatch(int which)
{
struct cpu_info *ci = curcpu();
- struct x86_soft_intr *si = &x86_soft_intrs[which];
+ struct x86_soft_intr_queue *queue = &softintr_queue[which];
struct x86_soft_intrhand *sih;
int floor;
floor = ci->ci_handled_intr_level;
ci->ci_handled_intr_level = ci->ci_ilevel;
- KERNEL_LOCK();
- for (;;) {
- mtx_enter(&si->softintr_lock);
- sih = TAILQ_FIRST(&si->softintr_q);
- if (sih == NULL) {
- mtx_leave(&si->softintr_lock);
- break;
+ mtx_enter(&softintr_lock);
+ while ((sih = TAILQ_FIRST(queue)) != NULL) {
+ KASSERT((sih->sih_state & (SIS_PENDING | SIS_RESTART)) ==
+ SIS_PENDING);
+ KASSERT(sih->sih_runner == NULL);
+
+ sih->sih_state &= ~SIS_PENDING;
+ TAILQ_REMOVE(queue, sih, sih_q);
+ sih->sih_runner = ci;
+ mtx_leave(&softintr_lock);
+
+ if (sih->sih_flags & SIF_MPSAFE) {
+ (*sih->sih_fn)(sih->sih_arg);
+ } else {
+ KERNEL_LOCK();
+ (*sih->sih_fn)(sih->sih_arg);
+ KERNEL_UNLOCK();
}
- TAILQ_REMOVE(&si->softintr_q, sih, sih_q);
- sih->sih_pending = 0;
- uvmexp.softs++;
-
- mtx_leave(&si->softintr_lock);
+ mtx_enter(&softintr_lock);
+ KASSERT((sih->sih_state & SIS_PENDING) == 0);
+ sih->sih_runner = NULL;
+ if (sih->sih_state & SIS_RESTART) {
+ TAILQ_INSERT_TAIL(queue, sih, sih_q);
+ sih->sih_state |= SIS_PENDING;
+ sih->sih_state &= ~SIS_RESTART;
+ }
- (*sih->sih_fn)(sih->sih_arg);
+ uvmexp.softs++;
}
- KERNEL_UNLOCK();
+ mtx_leave(&soft
Re: gprof: Profiling a multi-threaded application
I updated my patch. I see `struct gmonparam` must be same between the kernel and
kgmon(1). I deleted #ifndef in the `struct gmonparam`.
Is it OK?
diff --git a/lib/libc/gmon/gmon.c b/lib/libc/gmon/gmon.c
index 1ce0a1c289e..6887f4f5987 100644
--- a/lib/libc/gmon/gmon.c
+++ b/lib/libc/gmon/gmon.c
@@ -42,6 +42,16 @@
struct gmonparam _gmonparam = { GMON_PROF_OFF };
+#ifndef _KERNEL
+#include
+
+SLIST_HEAD(, gmonparam) _gmonfree = SLIST_HEAD_INITIALIZER(_gmonfree);
+SLIST_HEAD(, gmonparam) _gmoninuse = SLIST_HEAD_INITIALIZER(_gmoninuse);
+pthread_mutex_t _gmonlock = PTHREAD_MUTEX_INITIALIZER;
+pthread_key_t _gmonkey;
+struct gmonparam _gmondummy;
+#endif
+
static int s_scale;
/* see profil(2) where this is describe (incorrectly) */
#defineSCALE_1_TO_10x1L
@@ -52,6 +62,13 @@ PROTO_NORMAL(moncontrol);
PROTO_DEPRECATED(monstartup);
static int hertz(void);
+#ifndef _KERNEL
+static void _gmon_destructor(void *);
+struct gmonparam *_gmon_alloc(void);
+static void _gmon_merge(void);
+static void _gmon_merge_two(struct gmonparam *, struct gmonparam *);
+#endif
+
void
monstartup(u_long lowpc, u_long highpc)
{
@@ -114,6 +131,11 @@ monstartup(u_long lowpc, u_long highpc)
} else
s_scale = SCALE_1_TO_1;
+#ifndef _KERNEL
+ _gmondummy.state = GMON_PROF_BUSY;
+ pthread_key_create(&_gmonkey, _gmon_destructor);
+#endif
+
moncontrol(1);
return;
@@ -134,6 +156,194 @@ mapfailed:
}
__strong_alias(_monstartup,monstartup);
+#ifndef _KERNEL
+static void
+_gmon_destructor(void *arg)
+{
+ struct gmonparam *p = arg, *q, **prev;
+
+ if (p == &_gmondummy)
+ return;
+
+ pthread_setspecific(_gmonkey, &_gmondummy);
+
+ pthread_mutex_lock(&_gmonlock);
+ SLIST_REMOVE(&_gmoninuse, p, gmonparam, next);
+ SLIST_INSERT_HEAD(&_gmonfree, p, next);
+ pthread_mutex_unlock(&_gmonlock);
+
+ pthread_setspecific(_gmonkey, NULL);
+}
+
+struct gmonparam *
+_gmon_alloc(void)
+{
+ void *addr;
+ struct gmonparam *p;
+
+ pthread_mutex_lock(&_gmonlock);
+ p = SLIST_FIRST(&_gmonfree);
+ if (p != NULL) {
+ SLIST_REMOVE_HEAD(&_gmonfree, next);
+ SLIST_INSERT_HEAD(&_gmoninuse, p ,next);
+ } else {
+ pthread_mutex_unlock(&_gmonlock);
+ p = mmap(NULL, sizeof (struct gmonparam),
+PROT_READ|PROT_WRITE, MAP_ANON|MAP_PRIVATE, -1, 0);
+ if (p == MAP_FAILED)
+ goto mapfailed_2;
+ *p = _gmonparam;
+ p->kcount = NULL;
+ p->kcountsize = 0;
+ p->froms = NULL;
+ p->tos = NULL;
+ addr = mmap(NULL, p->fromssize, PROT_READ|PROT_WRITE,
+ MAP_ANON|MAP_PRIVATE, -1, 0);
+ if (addr == MAP_FAILED)
+ goto mapfailed;
+ p->froms = addr;
+
+ addr = mmap(NULL, p->tossize, PROT_READ|PROT_WRITE,
+ MAP_ANON|MAP_PRIVATE, -1, 0);
+ if (addr == MAP_FAILED)
+ goto mapfailed;
+ p->tos = addr;
+ pthread_mutex_lock(&_gmonlock);
+ SLIST_INSERT_HEAD(&_gmoninuse, p ,next);
+ }
+ pthread_mutex_unlock(&_gmonlock);
+ pthread_setspecific(_gmonkey, p);
+
+ return p;
+
+mapfailed:
+ if (p->froms != NULL) {
+ munmap(p->froms, p->fromssize);
+ p->froms = NULL;
+ }
+ if (p->tos != NULL) {
+ munmap(p->tos, p->tossize);
+ p->tos = NULL;
+ }
+mapfailed_2:
+ pthread_setspecific(_gmonkey, NULL);
+ ERR("_gmon_alloc: out of memory\n");
+ return NULL;
+}
+
+static void
+_gmon_merge_two(struct gmonparam *p, struct gmonparam *q)
+{
+ u_long fromindex;
+ u_short *frompcindex, qtoindex, toindex;
+ u_long selfpc;
+ u_long endfrom;
+ long count;
+ struct tostruct *top;
+
+ endfrom = (q->fromssize / sizeof(*q->froms));
+ for (fromindex = 0; fromindex < endfrom; fromindex++) {
+ if (q->froms[fromindex] == 0)
+ continue;
+ for (qtoindex = q->froms[fromindex]; qtoindex != 0;
+qtoindex = q->tos[qtoindex].link) {
+ selfpc = q->tos[qtoindex].selfpc;
+ count = q->tos[qtoindex].count;
+ /* cribbed from mcount */
+ frompcindex = &p->froms[fromindex];
+ toindex = *frompcindex;
+ if (toindex == 0) {
+ /*
+* first time traversing this arc
+*/
+ toindex = ++p->tos[0].link;
+ if (toindex >= p->tolimit)
+
