ipsec ipo tdb mutex
Hi,
To cache lookups, the policy ipo is linked to its SA tdb. There
is a list of SAs that belong to a policy. To make it MP safe we
need a mutex around these pointers.
Hrvoje: Can you test this alone and together with the ip forwarding
diff. I protects the data structure where the latter crashed.
Wonder if this helps.
ok?
bluhm
Index: net/pfkeyv2.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfkeyv2.c,v
retrieving revision 1.227
diff -u -p -r1.227 pfkeyv2.c
--- net/pfkeyv2.c 8 Dec 2021 14:24:18 - 1.227
+++ net/pfkeyv2.c 10 Dec 2021 17:26:25 -
@@ -2004,12 +2004,15 @@ pfkeyv2_send(struct socket *so, void *me
(caddr_t)&ipo->ipo_mask, rnh,
ipo->ipo_nodes, 0)) == NULL) {
/* Remove from linked list of policies on TDB */
+ mtx_enter(&ipo_tdb_mtx);
if (ipo->ipo_tdb != NULL) {
TAILQ_REMOVE(
&ipo->ipo_tdb->tdb_policy_head,
ipo, ipo_tdb_next);
tdb_unref(ipo->ipo_tdb);
+ ipo->ipo_tdb = NULL;
}
+ mtx_leave(&ipo_tdb_mtx);
if (ipo->ipo_ids)
ipsp_ids_free(ipo->ipo_ids);
pool_put(&ipsec_policy_pool, ipo);
Index: netinet/ip_ipsp.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.263
diff -u -p -r1.263 ip_ipsp.c
--- netinet/ip_ipsp.c 8 Dec 2021 14:24:18 - 1.263
+++ netinet/ip_ipsp.c 10 Dec 2021 17:26:25 -
@@ -926,12 +926,14 @@ tdb_cleanspd(struct tdb *tdbp)
{
struct ipsec_policy *ipo;
+ mtx_enter(&ipo_tdb_mtx);
while ((ipo = TAILQ_FIRST(&tdbp->tdb_policy_head)) != NULL) {
TAILQ_REMOVE(&tdbp->tdb_policy_head, ipo, ipo_tdb_next);
tdb_unref(ipo->ipo_tdb);
ipo->ipo_tdb = NULL;
ipo->ipo_last_searched = 0; /* Force a re-search. */
}
+ mtx_leave(&ipo_tdb_mtx);
}
void
Index: netinet/ip_ipsp.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.h,v
retrieving revision 1.229
diff -u -p -r1.229 ip_ipsp.h
--- netinet/ip_ipsp.h 8 Dec 2021 14:24:18 - 1.229
+++ netinet/ip_ipsp.h 10 Dec 2021 17:26:25 -
@@ -257,6 +257,10 @@ struct ipsec_acquire {
TAILQ_ENTRY(ipsec_acquire) ipa_next;
};
+/*
+ * Locks used to protect struct members in this file:
+ * p ipo_tdb_mtx link policy to TDB global mutex
+ */
struct ipsec_policy {
struct radix_node ipo_nodes[2]; /* radix tree glue */
struct sockaddr_encap ipo_addr;
@@ -274,7 +278,7 @@ struct ipsec_policy {
* mode was used.
*/
- u_int64_t ipo_last_searched; /* Timestamp of last
lookup */
+ u_int64_t ipo_last_searched; /* [p] Timestamp of lookup */
u_int8_tipo_flags; /* See IPSP_POLICY_*
definitions */
u_int8_tipo_type; /* USE/ACQUIRE/... */
@@ -283,7 +287,7 @@ struct ipsec_policy {
int ipo_ref_count;
- struct tdb *ipo_tdb; /* Cached entry */
+ struct tdb *ipo_tdb; /* [p] Cached TDB entry */
struct ipsec_ids*ipo_ids;
@@ -313,7 +317,8 @@ struct ipsec_policy {
* Locks used to protect struct members in this file:
* I immutable after creation
* N net lock
- * s tdb_sadb_mtx
+ * p ipo_tdb_mtx link policy to TDB global mutex
+ * s tdb_sadb_mtxSA database global mutex
*/
struct tdb { /* tunnel descriptor block */
/*
@@ -436,7 +441,7 @@ struct tdb {/* tunnel
descriptor blo
struct sockaddr_encap tdb_filter; /* What traffic is acceptable */
struct sockaddr_encap tdb_filtermask; /* And the mask */
- TAILQ_HEAD(tdb_policy_head, ipsec_policy) tdb_policy_head;
+ TAILQ_HEAD(tdb_policy_head, ipsec_policy) tdb_policy_head; /* [p] */
TAILQ_ENTRY(tdb)tdb_sync_entry;
};
#define tdb_ipackets tdb_data.tdd_ipackets
@@ -544,6 +549,7 @@ extern char ipsec_def_comp[];
extern TAILQ_HEAD(ipsec_policy_head, ipsec_policy) ipsec_policy_head;
extern struct mutex tdb_sadb_mtx;
+extern struct mutex ipo_tdb_mtx;
Re: gprof: Profiling a multi-threaded application
On 10/12/21(Fri) 09:56, Yuichiro NAITO wrote: > Any comments about this topic? I'm ok with this approach. I would appreciate if somebody else could take it over, I'm too busy with other stuff. > On 7/12/21 18:09, Yuichiro NAITO wrote: > > Hi, Martin > > > > n 2021/07/10 16:55, Martin Pieuchot wrote: > > > Hello Yuichiro, thanks for your work ! > > > > Thanks for the response. > > > > > > On 2021/06/16 16:34, Yuichiro NAITO wrote: > > > > > When I compile a multi-threaded application with '-pg' option, I > > > > > always get no > > > > > results in gmon.out. With bad luck, my application gets SIGSEGV while > > > > > running. > > > > > Because the buffer that holds number of caller/callee count is the > > > > > only one > > > > > in the process and will be broken by multi-threaded access. > > > > > > > > > > I get the idea to solve this problem from NetBSD. NetBSD has > > > > > individual buffers > > > > > for each thread and merges them at the end of profiling. > > > > > > Note that the kernel use a similar approach but doesn't merge the buffer, > > > instead it generates a file for each CPU. > > > > Yes, so the number of output files are limited by the number of CPUs in > > case of > > the kernel profiling. I think number of application threads can be > > increased more > > casually. I don't want to see dozens of 'gmon.out' files. > > > > > > > NetBSD stores the reference to the individual buffer by > > > > > pthread_setspecific(3). > > > > > I think it causes infinite recursive call if whole libc library > > > > > (except > > > > > mcount.c) is compiled with -pg. > > > > > > > > > > The compiler generates '_mcount' function call at the beginning of > > > > > every > > > > > functions. If '_mcount' calls pthread_getspecific(3) to get the > > > > > individual > > > > > buffer, pthread_getspecific() calls '_mcount' again and causes > > > > > infinite > > > > > recursion. > > > > > > > > > > NetBSD prevents from infinite recursive call by checking a global > > > > > variable. But > > > > > this approach also prevents simultaneously call of '_mcount' on a > > > > > multi-threaded > > > > > application. It makes a little bit wrong results of profiling. > > > > > > > > > > So I added a pointer to the buffer in `struct pthread` that can be > > > > > accessible > > > > > via macro call as same as pthread_self(3). This approach can prevent > > > > > of > > > > > infinite recursive call of '_mcount'. > > > > > > Not calling a libc function for this makes sense. However I'm not > > > convinced that accessing `tib_thread' before _rthread_init() has been > > > called is safe. > > > > Before '_rthread_init’ is called, '__isthreaded' global variable is kept to > > be 0. > > My patch doesn't access tib_thread in this case. > > After calling `_rthread_init`, `pthread_create()` changes `__isthreaded` to > > 1. > > Tib_thread will be accessed by all threads that are newly created and the > > initial one. > > > > I believe tib of the initial thread has been initialized in `_libc_preinit' > > function > > in 'lib/libc/dlfcn/init.c'. > > > > > I'm not sure where is the cleanest way to place the per-thread buffer, > > > I'd suggest you ask guenther@ about this. > > > > I added guenther@ in CC of this mail. > > I hope I can get an advise about per-thread buffer. > > > > > Maybe the initialization can be done outside of _mcount()? > > > > Yes, I think tib is initialized in `pthread_create()` and `_libc_preinit()`. > > > > > > > I obtained merging function from NetBSD that is called in '_mcleanup' > > > > > function. > > > > > Merging function needs to walk through all the individual buffers, > > > > > I added SLIST_ENTRY member in 'struct gmonparam' to make a list of > > > > > the buffers. > > > > > And also added '#ifndef _KERNEL' for the SLIST_ENTRY member not to be > > > > > used for > > > > > the kernel. > > > > > > > > > > But I still use pthread_getspecific(3) for that can call destructor > > > > > when > > > > > a thread is terminated. The individual buffer is moved to free list > > > > > to reuse > > > > > for a new thread. > > > > > > > > Here is a patch for this approach. > > > > > > I have various comments: > > > > > > We choose not to use C++ style comment (// comment) in the tree, could you > > > fix yours? > > > > > > There's two copies of mcount.c, the other one lies in sys/lib/libkern > > > could > > > you keep them in sync? > > > > > > gmon.c is only compiled in userland and don't need #ifndef _KERNEL > > > > > > In libc there's also the use of _MUTEX_LOCK/UNLOCK() macro instead of > > > calling pthread_mutex* directly. This might help reduce the differences > > > between ST and MT paths. > > > > Thanks for letting me know them. > > I updated my patch as following according to the advice. > > > > diff --git a/lib/libc/gmon/gmon.c b/lib/libc/gmon/gmon.c > > index 1ce0a1c289e..5169fa70449 100644 > > --- a/lib/libc/gmon/gmon.c > > +++ b/lib/libc/gmon/gmon.c > >
Re: Fix typo in '}' command in less.1
Hi Richard,
Richard Ulmer wrote on Fri, Dec 10, 2021 at 04:04:11PM +0100:
> this is just a minor copy-and-paste error fix for the less(1) man page.
Committed, thank you.
> I also contributed this upstream: https://github.com/gwsw/less/pull/228
>
> I hope this is the right mailing list for this.
Yes, it is.
Posting patches to tech@ is not required if, for a specific patch,
you know a better destination. But tech@ is appropriate for all
kinds of patches, even documentation, in particular when you are
unsure where to send them.
Yours,
Ingo
> Index: less.1
> ===
> RCS file: /cvs/src/usr.bin/less/less.1,v
> retrieving revision 1.58
> diff -u -p -u -r1.58 less.1
> --- less.1 23 Sep 2021 18:46:25 - 1.58
> +++ less.1 10 Dec 2021 14:58:46 -
> @@ -852,7 +852,7 @@ If a right curly bracket appears in the
> the } command will go to the matching left curly bracket.
> The matching left curly bracket is positioned on the top
> line of the screen.
> -If there is more than one right curly bracket on the top line,
> +If there is more than one right curly bracket on the bottom line,
> a number N may be used to specify the N-th bracket on the line.
> .It Ic \&(
> Like {, but applies to parentheses rather than curly brackets.
Re: sndiod: -F does not switch back to preferred device
Alexandre Ratchov wrote:
> On Thu, Dec 09, 2021 at 09:02:07PM -0700, Theo de Raadt wrote:
> >
> > I think drivers, or maybe this can be done in higher-level subsystems, need
> > to be modified such that events get sent when a device is *actually ready*,
> > and the call from config_attach() should be deleted.
> >
>
> For audio and MIDI, I think we're not very far from this.
>
> Currently, as soon as audio_attach() returns, both hardware and
> audio(4) internal structures are properly initialized. So all the
> audio_{open,read,write,ioctl,close}() interfaces are safe to call.
>
> What happens in higher layers is unclear to me? for instance, what
> needs to be done for open(2) to reach audioopen() with no races?
I think that misses the point.
If audio works, are you going to fix usb and scsi?
Being ready immediately at attach time isn't a specified behaviour.
We don't need luck.
Re: Make __EV_POLL flag specific to kqueue-based poll(2)
On Thu, 09 Dec 2021 15:38:36 +, Visa Hankala wrote: > The system now has two flags for poll and select system calls, > __EV_POLL which currently covers both calls, and __EV_SELECT which is > used with select only. > > To make the code easier to follow, this diff makes __EV_POLL specific > to poll(2). Consequently, __EV_SELECT has to be added when the condition > is relevant to select(2). __EV_HUP will be specific to kqueue-based > poll(2), so the code that sets __EV_HUP can keep on checking just > __EV_POLL. Yes, I think this is easier to understand. OK millert@ - todd
