Fixing sysctl LOR

[mdf at FreeBSD.org]

On Mon, Nov 29, 2010 at 5:11 AM, John Baldwin <jhb at freebsd.org> wrote:
> On Thursday, November 25, 2010 8:44:07 pm mdf at freebsd.org wrote:
>> My previous thought on the matter was incorrect.  This patch (and two
>> small cleanups before it) mean the sysctl lock is no longer held
>> across the call to oid_handler, which means that WITNESS will no
>> longer make entries where sysctl lock is taken before any random lock
>> in the handler.
>>
>> The idea is simple; keep track of the number of threads running the
>> handler so that the oid is not deleted (and sysctl_ctx_free(9) doesn't
>> return) before all threads are drained.  It does unfortunately mean
>> that the sysctl lock is  only taken in exclusive mode, but since it's
>> held for less time I don't anticipate a significant loss of
>> concurrency.  If there is a simple benchmark someone can recommend I'd
>> be happy to check the difference.
>>
>> I would like at some point to also reduce the number of calls to
>> sysctl(2) made by sysctl(8); this would also help performance.  Among
>> other things I wonder if eliminating the numerical array to describe
>> an oid would be acceptable; in a few circumstances it would mean
>> longer comparisons (strcmp versus integer comparison) but for many
>> uses it eliminates the name2oid step, and it would also mean there's
>> no longer a need for fixed numbered entries.
>>
>> Cleanup:
>> http://people.freebsd.org/~mdf/0001-Use-the-SYSCTL_CHILDREN-macro-in-
> kern_sysctl.c-to-he.patch
>> http://people.freebsd.org/~mdf/0002-Slightly-modify-the-logic-in-
> sysctl_find_oid-to-redu.patch
>
> These look fine to me.
>
>> The patch:
>> http://people.freebsd.org/~mdf/0003-Do-not-hold-the-sysctl-lock-across-a-
> call-to-the-han.patch
>
> Concurrent sysctl's aren't that big of a gain anyway, especially since it only
> worked for in-kernel invocations (very rare) and not for userland invocations.
>
> Minor nit:
>
> --- a/sys/sys/sysctl.h
> +++ b/sys/sys/sysctl.h
> @@ -87,7 +87,7 @@ struct ctlname {
>  #define CTLFLAG_MPSAFE 0x00040000      /* Handler is MP safe */
>  #define CTLFLAG_VNET   0x00020000      /* Prisons with vnet can fiddle */
>  #define CTLFLAG_RDTUN  (CTLFLAG_RD|CTLFLAG_TUN)
> -
> +#define        CTLFLAG_DYING   0x00010000      /* oid is being removed */
>  /*
>  * Secure level.   Note that CTLFLAG_SECURE == CTLFLAG_SECURE1.
>  *
>
> The newline before the block comment should stay.
>
> Also, this isn't MFC'able since it breaks the KBI.  If you wanted to MFC I
> suppose you could break the oid_refcnt into two shorts for the MFC patch (but
> keep it as full int's in HEAD).

I wasn't sure it was useful enough a change to be MFC'd, since few
people seem to have reported issues, much less full deadlocks like we
have at Isilon due to our increased use of sysctl oid's for everything
under the sun.

Thanks,
matthew