Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 24 Aug 2017 13:43:47 -0600
From:      Alan Somers <asomers@freebsd.org>
To:        Bruce Evans <brde@optusnet.com.au>
Cc:        "src-committers@freebsd.org" <src-committers@freebsd.org>,  "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>,  "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>
Subject:   Re: svn commit: r322258 - head/sys/kern
Message-ID:  <CAOtMX2jS2SdN9mgbu0E_9WytCMFYUeZ6RPg=EGcAFe%2BBj17=TA@mail.gmail.com>
In-Reply-To: <20170809141608.I1096@besplex.bde.org>
References:  <201708081614.v78GEVGY066448@repo.freebsd.org> <20170809141608.I1096@besplex.bde.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Aug 9, 2017 at 1:05 AM, Bruce Evans <brde@optusnet.com.au> wrote:
> On Tue, 8 Aug 2017, Alan Somers wrote:
>
>> Log:
>>  Make p1003_1b.aio_listio_max a tunable
>>
>>  p1003_1b.aio_listio_max is now a tunable. Its value is reflected in the
>>  sysctl of the same name, and the sysconf(3) variable _SC_AIO_LISTIO_MAX.
>>  Its value will be bounded from below by the compile-time constant
>>  AIO_LISTIO_MAX and from above by the compile-time constant
>>  MAX_AIO_QUEUE_PER_PROC and the tunable vfs.aio.max_aio_queue.
>
>
> I don't like tunables or sysctls being limited or silently limiting them.
> This is not done for more important sysctls like kern.maxfilesperproc.
> Lots of silent runtime limiting is done for a few more important limits
> like kern.maxfilesperproc.
>
> The limiting is also buggy:
> - vfs.aio.max_aio_queue isn't a tunable as stated.  It is a r/w sysctl
> - if vfs.aio.max_aio_queue were a r/o tunable, then there would be a
>   solvable ordering problem initializing it before using it
> - since vfs.aio.max_aio_queue is only a sysctl, it is unusable at boot
>   time (and module load time), but it is used there.
>
> Other bugs in vfs.aio.max_aio_queue:
> - its name is too long, yet not very descriptive
> - its name repeats "aio"
> - its name but doesn't say "len", so it looks like it limits a number of
>   queues and not the length of a single queue.  This is backwards relative
>   to the corresponding variable name.  That is missing "aio" when it
>   is needed, but only spells "length" badly as "count".  It is
>   max_queue_count, but should be something like aio_max_qlen.  All
>   sysctl variables should have an aio_ prefix which is removed in the
>   leaf name in the sysctl tree.
> Other names have similar or worse bugs.  Bugs start with parameter names
> MAX_AIO_* instead of AIO_*_MAX, and variable names track this.  There is
> some ambiguity from global vs per-process limits and counts, and the
> noise word "num" is sprinkled to get names which differ without describing
> any difference.
>
>> Modified: head/sys/kern/vfs_aio.c
>>
>> ==============================================================================
>> --- head/sys/kern/vfs_aio.c     Tue Aug  8 16:06:16 2017        (r322257)
>> +++ head/sys/kern/vfs_aio.c     Tue Aug  8 16:14:31 2017        (r322258)
>> @@ -102,6 +102,7 @@ static uint64_t jobseqno;
>> #endif
>>
>> FEATURE(aio, "Asynchronous I/O");
>> +SYSCTL_DECL(_p1003_1b);
>>
>> static MALLOC_DEFINE(M_LIO, "lio", "listio aio control block list");
>>
>> @@ -168,6 +169,11 @@ static int max_buf_aio = MAX_BUF_AIO;
>> SYSCTL_INT(_vfs_aio, OID_AUTO, max_buf_aio, CTLFLAG_RW, &max_buf_aio, 0,
>>     "Maximum buf aio requests per process (stored in the process)");
>>
>> +static int aio_listio_max = AIO_LISTIO_MAX;
>> +SYSCTL_INT(_p1003_1b, CTL_P1003_1B_AIO_LISTIO_MAX, aio_listio_max,
>> +    CTLFLAG_RDTUN | CTLFLAG_CAPRD, &aio_listio_max, 0,
>> +    "Maximum aio requests for a single lio_listio call");
>> +
>
>
> The POSIX namespace is missing all of the bugs described above.  It has
> aio first (except, for historical reasons it unfortunately has p10031b
> instead of vfs.aio for the sysctl prefix), and max last.
>
>> #ifdef COMPAT_FREEBSD6
>> typedef struct oaiocb {
>>         int     aio_fildes;             /* File descriptor */
>> @@ -388,6 +394,11 @@ static int
>> aio_onceonly(void)
>> {
>>
>> +       if (aio_listio_max < AIO_LISTIO_MAX)
>> +               aio_listio_max = AIO_LISTIO_MAX;
>> +       if (aio_listio_max > MIN(MAX_AIO_QUEUE_PER_PROC, max_queue_count))
>> +               aio_listio_max = MIN(MAX_AIO_QUEUE_PER_PROC,
>> max_queue_count);
>> +
>
>
> max_queue_count is not initialized here, except to a default at compile
> time.  Except for module (re)load, it always has the default value
> MAX_AIO_QUEUE = 1024
>   (this 1024 is bogusly ifdefed.  MAX_AIO_QUEUE is not defined in any
>   header file and is not a supported option, and the tunable makes the
>   option less needed than before).
> MAX_AIO_QUEUE_PER_PROC = 256, modulo a similar unsupported option.
> Thus MIN(MAX_AIO_QUEUE_PER_PROC, max_queue_count) is an obfuscated spelling
> of 256.
>
> MAX_AIO_QUEUE_PER_PROC is also just the default for a variable
> (max_aio_queue_per_proc), so using it here has the same bugs except for
> the obfuscation.
>
> Using MIN() instead of imin() is a style bug.
>
> OTOH, AIO_LISTIO_MAX is defined (as 16) in a header file, and is not
> bogusly ifdefed.  It is 1 of 3 aio limits specified by POSIX.
>
> The checking would be buggy if it were done later, since max_queue_count
> is not checked and limiting to it can corrupt aio_listio_max (when it is
> negative or just too small).
>
> The compile-time definition of AIO_LISTIO_MAX seems to be broken.  I think
> POSIX species that AIO_LISTIO_MAX shall not be defined if the value of
> {AIO_LISTIO_MAX} varies at runtime, but it is still defined.  FreeBSD
> has this bug for many other sysconf() variables, e.g., {OPEN_MAX}.
> Perhaps AIO_LISTIO_MAX is easier to fix since it is not hard-coded as
> often as OPEN_MAX.

What you describe is Linux's behavior, but the POSIX requirement is a
bit more general.  All POSIX says is that "The value returned [by
sysconf(3)] shall not be more restrictive than the corresponding value
described to the application when it was compiled with the
implementation's <limits.h> or <unistd.h>".

>
> AIO_LISTIO_MAX is now just the FreeBSD default, but the first check in
> the above restricts {AIO_LISTIO_MAX} to this default (16) instead of to
> the POSIX limit of _POSIX_IO_LISTIO_MAX (2).  Relaxing this check would
> cause more problems with hard-coded AIO_LISTIO_MAX.
>
> If the second check in the above were done at runtime so that it worked
> for changed max_queue_count, then it would reduce aio_listio_max below
> the POSIX limit of 2 slightly before it reduces to garbage zero and negative
> values.  The first check should be large, or if you want to give a
> preference
> for >= the old limit of 16 and a hard limit of the POSIX limit of 2, check
> keep checking the old limit first and the POSIX limit last.
>
>>         exit_tag = EVENTHANDLER_REGISTER(process_exit, aio_proc_rundown,
>> NULL,
>>             EVENTHANDLER_PRI_ANY);
>>         exec_tag = EVENTHANDLER_REGISTER(process_exec,
>> aio_proc_rundown_exec,
>> @@ -405,14 +416,13 @@ aio_onceonly(void)
>>             NULL, NULL, NULL, UMA_ALIGN_PTR, UMA_ZONE_NOFREE);
>>         aiocb_zone = uma_zcreate("AIOCB", sizeof(struct kaiocb), NULL,
>> NULL,
>>             NULL, NULL, UMA_ALIGN_PTR, UMA_ZONE_NOFREE);
>> -       aiol_zone = uma_zcreate("AIOL", AIO_LISTIO_MAX*sizeof(intptr_t) ,
>> NULL,
>> -           NULL, NULL, NULL, UMA_ALIGN_PTR, UMA_ZONE_NOFREE);
>> +       aiol_zone = uma_zcreate("AIOL", aio_listio_max * sizeof(intptr_t)
>> ,
>> +           NULL, NULL, NULL, NULL, UMA_ALIGN_PTR, UMA_ZONE_NOFREE);
>
>
> Allocations like this are hard to adjust later (I forget if the size limit
> is hard or soft, but it should be hard).  The user must be allowed to set
> aio_listio_max to value larger than the default, so as to prepare for
> expansion of the other limits later.  However, this doesn't work, since
> it leaves the other limits inconsistent until they are expanded.
>
> It seems best to adjust all the other limits based on the aio_listio_max
> tunable.  Usually this is not set.  Use the old parameters then, but remove
> all the macros and compile-time initializers for them.  Do onceonly
> initialization like
>
>         if (aio_listio_max == 16) {
>                 max_queue_count = 1024; /* historical MAX_AIO_QUEUE */
>                 ...
>         }
>
> When aio_listio_max != 16, calculate the parameters somehow.  I don't know
> how to do this.  At least make them consistent.
>
> After initialization, the other parameters are still restricted by the
> choice for aio_listio_max.  Leave it to the user to not choose bad
> combinations.
>
> The aio sysctls seem to already break many of the POSIX sysctls.  E.g.,
> MAX_AIO_QUEUE is used as the default for the r/w sysctl
> vfs.aio.max_aio_queue and as the value for the sysctl r/o sysctl
> p1003_1b.aio_max.  Any change to the first sysctl breaks the second
> sysctl.
>
> The 3 POSIX (2001) aio sysctls thus have quite different invariance
> bugs:
> - {AIO_MAX}: AIO_MAX is correctly not defined.  {AIO_MAX} is variable
>   by a low-level sysctl but the p1003 sysctl used to report {AIO_MAX}
>   doesn't see the change.
> - {AIO_PRIO_DELTA_MAX}: no low-level sysctl or variable to break it
>   (value is always 0)
> - {AIO_LISTIO_MAX}: was constant until this commit.  There is not
>   low-level sysctl for it, and the low-level variable for it is
>   correctly exported, but AIO_LISTIO_MAX was incorrectly defined
>   so it is hard to change now.  {AIO_LISTIO_MAX} is inconsistent
>   with the macro whenever the tunable is used to change its default.
>
> Bruce

I dug deeper and found that there wasn't any good reason for the
aio_listio_max limit to exist in the first place.  This DR eliminates
it, which I think will satisfy most of your concerns.
https://reviews.freebsd.org/D12120

-Alan



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2jS2SdN9mgbu0E_9WytCMFYUeZ6RPg=EGcAFe%2BBj17=TA>