From owner-freebsd-arch@FreeBSD.ORG Sat Oct 25 14:16:03 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 4443DA17 for ; Sat, 25 Oct 2014 14:16:03 +0000 (UTC) Received: from mail-wg0-x232.google.com (mail-wg0-x232.google.com [IPv6:2a00:1450:400c:c00::232]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id C43D5BFE for ; Sat, 25 Oct 2014 14:16:02 +0000 (UTC) Received: by mail-wg0-f50.google.com with SMTP id z12so302709wgg.9 for ; Sat, 25 Oct 2014 07:16:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=2cX1r8SY89mZ5Qlcy73rZDdEb7vhJHXuH2+njkv4XBk=; b=Z5vCwre8FWloMyKxZ5dV7wuV7oaMi5eHJWyUq4w6EGaAywlCstvoiOsg8AC61EbVHz a9r7z7Dok1/TMdgMXGXy0kjqHDOt9llTDE32D0fWprS4+Q36gjAymCCaV09CqHnu94+P btqsLkixezPmsdxQBfUgDF/rXPDZuAR3d/iW47gb6hIseBU+Alpteu2s0+ObXhhrt4Gv g0VMB16IPbGVSmWtaE22RMA5iLu9OPtVpzZnEMr/ZG9Aveg+S0D4Mk4eKV6QLttA6Mqe RjTdn6vmf/QnzuWJ5OlJmYQdYPAua9hQ7LHKWUqbCiHScsKO1igBF2rQQGZxMGOS2aJn NyOw== X-Received: by 10.180.149.208 with SMTP id uc16mr10364962wib.23.1414246560887; Sat, 25 Oct 2014 07:16:00 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id fq1sm5243041wib.12.2014.10.25.07.15.59 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 25 Oct 2014 07:16:00 -0700 (PDT) Date: Sat, 25 Oct 2014 16:15:57 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: syscalls from loadable modules compiled in statically into the kernel Message-ID: <20141025141557.GB20599@dft-labs.eu> References: <20141025022808.GA14551@dft-labs.eu> <20141025092234.GI1877@kib.kiev.ua> <20141025132039.GA20599@dft-labs.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20141025132039.GA20599@dft-labs.eu> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 25 Oct 2014 14:16:03 -0000 On Sat, Oct 25, 2014 at 03:20:39PM +0200, Mateusz Guzik wrote: > On Sat, Oct 25, 2014 at 12:22:34PM +0300, Konstantin Belousov wrote: > > On Sat, Oct 25, 2014 at 04:28:09AM +0200, Mateusz Guzik wrote: > > > The kernel has the following mechanism: > > > > > > int > > > syscall_thread_enter(struct thread *td, struct sysent *se) > > > { > > > u_int32_t cnt, oldcnt; > > > > > > do { > > > oldcnt = se->sy_thrcnt; > > > if ((oldcnt & SY_THR_STATIC) != 0) > > > return (0); > > > if ((oldcnt & (SY_THR_DRAINING | SY_THR_ABSENT)) != 0) > > > return (ENOSYS); > > > cnt = oldcnt + SY_THR_INCR; > > > } while (atomic_cmpset_acq_32(&se->sy_thrcnt, oldcnt, cnt) == 0); > > > return (0); > > > } > > > > > > Except it turns out that it is used even if given module (here: sysvshm) is > > > compiled in statically. > > > > > > So my proposal is to give modules an easy way to tell whether they got > > > compiled in and extend syscall_register interface so that it would allow > > > registering static syscalls. > > > > > > The latter could also be used by modules which are loadable, but don't > > > support unloads. > > > > > > I don't have any good idea how to provide aforementioned detection > > > method though. > > The method would be a combination of some change to syscall_register() > > and #ifdef KLD_MODULE. Look at the sys/conf.h MAKEDEV_ETERNAL_KLD > > definition, which provides similar in spirit optimization for > > non-destructable cdevs. > > > > Ok, so I'll add sysctl_register_flags and SY_THR_STATIC_KLD + making > sure SY_THR_STATIC cannot be unregistered. > Turns out freebsd32 duplicates a lot of code and didn't receive some fixes regular syscall table support got. I decided to just patch it up without fixing that for now. diff --git a/sys/compat/freebsd32/freebsd32_misc.c b/sys/compat/freebsd32/freebsd32_misc.c index d909a71..8fe4b83 100644 --- a/sys/compat/freebsd32/freebsd32_misc.c +++ b/sys/compat/freebsd32/freebsd32_misc.c @@ -2627,9 +2627,13 @@ freebsd32_xxx(struct thread *td, struct freebsd32_xxx_args *uap) #endif int -syscall32_register(int *offset, struct sysent *new_sysent, - struct sysent *old_sysent) +syscall32_register_flags(int *offset, struct sysent *new_sysent, + struct sysent *old_sysent, int flags) { + + if ((flags & ~SY_THR_STATIC) != 0) + return (EINVAL); + if (*offset == NO_SYSCALL) { int i; @@ -2648,15 +2652,26 @@ syscall32_register(int *offset, struct sysent *new_sysent, *old_sysent = freebsd32_sysent[*offset]; freebsd32_sysent[*offset] = *new_sysent; + atomic_store_rel_32(&freebsd32_sysent[*offset].sy_thrcnt, flags); return 0; } int +syscall32_register(int *offset, struct sysent *new_sysent, + struct sysent *old_sysent) +{ + + return (syscall32_register_flags(offset, new_sysent, old_sysent, 0)); +} + +int syscall32_deregister(int *offset, struct sysent *old_sysent) { - if (*offset) - freebsd32_sysent[*offset] = *old_sysent; + if (*offset == 0) + return (0); + + freebsd32_sysent[*offset] = *old_sysent; return 0; } @@ -2707,14 +2722,14 @@ syscall32_module_handler(struct module *mod, int what, void *arg) } int -syscall32_helper_register(struct syscall_helper_data *sd) +syscall32_helper_register_flags(struct syscall_helper_data *sd, int flags) { struct syscall_helper_data *sd1; int error; for (sd1 = sd; sd1->syscall_no != NO_SYSCALL; sd1++) { - error = syscall32_register(&sd1->syscall_no, &sd1->new_sysent, - &sd1->old_sysent); + error = syscall32_register_flags(&sd1->syscall_no, + &sd1->new_sysent, &sd1->old_sysent, flags); if (error != 0) { syscall32_helper_unregister(sd); return (error); @@ -2725,6 +2740,13 @@ syscall32_helper_register(struct syscall_helper_data *sd) } int +syscall32_helper_register(struct syscall_helper_data *sd) +{ + + return (syscall32_helper_register_flags(sd, 0)); +} + +int syscall32_helper_unregister(struct syscall_helper_data *sd) { struct syscall_helper_data *sd1; diff --git a/sys/compat/freebsd32/freebsd32_util.h b/sys/compat/freebsd32/freebsd32_util.h index a5945cf..e473ef6 100644 --- a/sys/compat/freebsd32/freebsd32_util.h +++ b/sys/compat/freebsd32/freebsd32_util.h @@ -97,10 +97,14 @@ SYSCALL32_MODULE(syscallname, \ .syscall_no = FREEBSD32_SYS_##syscallname \ } +int syscall32_register_flags(int *offset, struct sysent *new_sysent, + struct sysent *old_sysent, int flags); int syscall32_register(int *offset, struct sysent *new_sysent, struct sysent *old_sysent); int syscall32_deregister(int *offset, struct sysent *old_sysent); int syscall32_module_handler(struct module *mod, int what, void *arg); +int syscall32_helper_register_flags(struct syscall_helper_data *sd, + int flags); int syscall32_helper_register(struct syscall_helper_data *sd); int syscall32_helper_unregister(struct syscall_helper_data *sd); diff --git a/sys/kern/kern_syscalls.c b/sys/kern/kern_syscalls.c index 03f6088..30dd203 100644 --- a/sys/kern/kern_syscalls.c +++ b/sys/kern/kern_syscalls.c @@ -104,11 +104,14 @@ syscall_thread_exit(struct thread *td, struct sysent *se) } int -syscall_register(int *offset, struct sysent *new_sysent, - struct sysent *old_sysent) +syscall_register_flags(int *offset, struct sysent *new_sysent, + struct sysent *old_sysent, int flags) { int i; + if ((flags & ~SY_THR_STATIC) != 0) + return (EINVAL); + if (*offset == NO_SYSCALL) { for (i = 1; i < SYS_MAXSYSCALL; ++i) if (sysent[i].sy_call == (sy_call_t *)lkmnosys) @@ -127,18 +130,31 @@ syscall_register(int *offset, struct sysent *new_sysent, *old_sysent = sysent[*offset]; new_sysent->sy_thrcnt = SY_THR_ABSENT; sysent[*offset] = *new_sysent; - atomic_store_rel_32(&sysent[*offset].sy_thrcnt, 0); + atomic_store_rel_32(&sysent[*offset].sy_thrcnt, flags); return (0); } int +syscall_register(int *offset, struct sysent *new_sysent, + struct sysent *old_sysent) +{ + + return (syscall_register_flags(offset, new_sysent, old_sysent, 0)); +} + +int syscall_deregister(int *offset, struct sysent *old_sysent) { + struct sysent *se; - if (*offset) { - syscall_thread_drain(&sysent[*offset]); - sysent[*offset] = *old_sysent; - } + if (*offset == 0) + return (0); /* XXX? */ + + se = &sysent[*offset]; + if ((se->sy_thrcnt & SY_THR_STATIC) != 0) + return (EINVAL); + syscall_thread_drain(se); + sysent[*offset] = *old_sysent; return (0); } @@ -190,14 +206,14 @@ syscall_module_handler(struct module *mod, int what, void *arg) } int -syscall_helper_register(struct syscall_helper_data *sd) +syscall_helper_register_flags(struct syscall_helper_data *sd, int flags) { struct syscall_helper_data *sd1; int error; for (sd1 = sd; sd1->syscall_no != NO_SYSCALL; sd1++) { - error = syscall_register(&sd1->syscall_no, &sd1->new_sysent, - &sd1->old_sysent); + error = syscall_register_flags(&sd1->syscall_no, + &sd1->new_sysent, &sd1->old_sysent, flags); if (error != 0) { syscall_helper_unregister(sd); return (error); @@ -208,6 +224,13 @@ syscall_helper_register(struct syscall_helper_data *sd) } int +syscall_helper_register(struct syscall_helper_data *sd) +{ + + return (syscall_helper_register_flags(sd, 0)); +} + +int syscall_helper_unregister(struct syscall_helper_data *sd) { struct syscall_helper_data *sd1; diff --git a/sys/kern/sysv_msg.c b/sys/kern/sysv_msg.c index a572a0e..4fc04bc 100644 --- a/sys/kern/sysv_msg.c +++ b/sys/kern/sysv_msg.c @@ -252,11 +252,12 @@ msginit() } mtx_init(&msq_mtx, "msq", NULL, MTX_DEF); - error = syscall_helper_register(msg_syscalls); + error = syscall_helper_register_flags(msg_syscalls, SY_THR_STATIC_KLD); if (error != 0) return (error); #ifdef COMPAT_FREEBSD32 - error = syscall32_helper_register(msg32_syscalls); + error = syscall32_helper_register_flags(msg32_syscalls, + SY_THR_STATIC_KLD); if (error != 0) return (error); #endif diff --git a/sys/kern/sysv_sem.c b/sys/kern/sysv_sem.c index c632902..dc1c66a 100644 --- a/sys/kern/sysv_sem.c +++ b/sys/kern/sysv_sem.c @@ -278,11 +278,12 @@ seminit(void) semexit_tag = EVENTHANDLER_REGISTER(process_exit, semexit_myhook, NULL, EVENTHANDLER_PRI_ANY); - error = syscall_helper_register(sem_syscalls); + error = syscall_helper_register_flags(sem_syscalls, SY_THR_STATIC_KLD); if (error != 0) return (error); #ifdef COMPAT_FREEBSD32 - error = syscall32_helper_register(sem32_syscalls); + error = syscall32_helper_register_flags(sem32_syscalls, + SY_THR_STATIC_KLD); if (error != 0) return (error); #endif diff --git a/sys/kern/sysv_shm.c b/sys/kern/sysv_shm.c index 3480d11..b3132c3 100644 --- a/sys/kern/sysv_shm.c +++ b/sys/kern/sysv_shm.c @@ -910,11 +910,12 @@ shminit() shmexit_hook = &shmexit_myhook; shmfork_hook = &shmfork_myhook; - error = syscall_helper_register(shm_syscalls); + error = syscall_helper_register_flags(shm_syscalls, SY_THR_STATIC_KLD); if (error != 0) return (error); #ifdef COMPAT_FREEBSD32 - error = syscall32_helper_register(shm32_syscalls); + error = syscall32_helper_register_flags(shm32_syscalls, + SY_THR_STATIC_KLD); if (error != 0) return (error); #endif diff --git a/sys/netinet/sctp_syscalls.c b/sys/netinet/sctp_syscalls.c index 3d0f549..8170ca8 100644 --- a/sys/netinet/sctp_syscalls.c +++ b/sys/netinet/sctp_syscalls.c @@ -94,11 +94,11 @@ sctp_syscalls_init(void *unused __unused) { int error; - error = syscall_helper_register(sctp_syscalls); + error = syscall_helper_register_flags(sctp_syscalls, SY_THR_STATIC); KASSERT((error == 0), ("%s: syscall_helper_register failed for sctp syscalls", __func__)); #ifdef COMPAT_FREEBSD32 - error = syscall32_helper_register(sctp_syscalls); + error = syscall32_helper_register_flags(sctp_syscalls, SY_THR_STATIC); KASSERT((error == 0), ("%s: syscall32_helper_register failed for sctp syscalls", __func__)); diff --git a/sys/sys/sysent.h b/sys/sys/sysent.h index 0f1c256..12bc518 100644 --- a/sys/sys/sysent.h +++ b/sys/sys/sysent.h @@ -76,6 +76,12 @@ struct sysent { /* system call table */ #define SY_THR_ABSENT 0x4 #define SY_THR_INCR 0x8 +#ifdef KLD_MODULE +#define SY_THR_STATIC_KLD 0 +#else +#define SY_THR_STATIC_KLD 0x1 +#endif + struct image_params; struct __sigset; struct syscall_args; @@ -241,10 +247,13 @@ struct syscall_helper_data { .syscall_no = NO_SYSCALL \ } +int syscall_register_flags(int *offset, struct sysent *new_sysent, + struct sysent *old_sysent, int flags); int syscall_register(int *offset, struct sysent *new_sysent, struct sysent *old_sysent); int syscall_deregister(int *offset, struct sysent *old_sysent); int syscall_module_handler(struct module *mod, int what, void *arg); +int syscall_helper_register_flags(struct syscall_helper_data *sd, int flags); int syscall_helper_register(struct syscall_helper_data *sd); int syscall_helper_unregister(struct syscall_helper_data *sd); > > > > > > Also, please see https://reviews.freebsd.org/D1007 which moves > > > SY_THR_STATIC check to an inline function, saving us 2 function calls on > > > each syscall. > > > > Did you benchmarked this ? I dislike the code bloat. > > with syscall_timing from tools/tools ministat says +4% for getuid and +1 > for pipe+close. > > -- > Mateusz Guzik -- Mateusz Guzik