Re: lwp resource limit

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

christos%zoulas.com@localhost (Christos Zoulas) wrote:
> Hello,
> 
> This is a new resource limit to prevent users from exhausting kernel
> resources that lwps use.
> 
> ...
> 
> comments?
> 

Few comments on the patch:

> Index: kern/init_main.c
> ===================================================================
> RCS file: /cvsroot/src/sys/kern/init_main.c,v
> retrieving revision 1.442
> diff -u -p -u -r1.442 init_main.c
> --- kern/init_main.c  19 Feb 2012 21:06:47 -0000      1.442
> +++ kern/init_main.c  23 May 2012 23:19:31 -0000
> @@ -256,6 +256,7 @@ int       cold = 1;                       /* still
> working on star struct timespec boottime;             /* time at
> system startup - will only follow settime deltas */ 
>  int  start_init_exec;                /* semaphore for start_init()
> */ +int       maxlwp;
> 

maxlwp should be __read_mostly.  However, why is it (and all sysctls)
in init_main.c?  I suppose you just followed current way (just historic
code), but I think it is a bad practice.  We should move such globals,
as well as sysctls, to the modules they belong to and make them static.
In this case it would be kern_lwp.c module.

> @@ -291,6 +292,12 @@ main(void)
>  #endif
>       l->l_pflag |= LP_RUNNING;
>  
> +#ifdef __HAVE_CPU_MAXLWP
> +     maxlwp = cpu_maxlwp();
> +#else
> +     maxlwp = 1024;
> +#endif
> +

1024 seems to small for me.  How about 2048 (if not 4096)?  Please make
it a #define somewhere (perhaps lwp.h?).  When would the MD code override
the value?

> +     if (nmaxlwp < 0 || nmaxlwp >= 65536)
> +             return EINVAL;
> +#ifdef __HAVE_CPU_MAXLWP
> +     if (nmaxlwp > cpu_maxlwp())
> +             return EINVAL;
> +#endif
> +     maxlwp = nmaxlwp;

If there are MD limitations, I tend to think that we should restructure
the code a little and allow lwp_create() to fail..

>  int
>  lwp_create(lwp_t *l1, proc_t *p2, vaddr_t uaddr, int flags,
>          void *stack, size_t stacksize, void (*func)(void *), void
> *arg,
> -        lwp_t **rnewlwpp, int sclass)
> +        lwp_t **rnewlwpp, int sclass, int enforce)
>  {

Variable 'enforce' should bool, but do we really need it?  We can know
whether it is a first LWP i.e. fork() call or creation of kthread from
inside.  Also, why not to account simple processes?  fork() is still a
creation of LWP.  Do you know how QNX accounts this?

For allowing process creation when maxlwp limit is exhausted, I would
say this problem is better solved with maxprocperuid you mentioned.

> +     uid = kauth_cred_getuid(l1->l_cred);
> +     count = chglwpcnt(uid, 1);

There is no need to perform chglwpcnt(), which has a cost of atomic
operation, if we are not checking against the limit (kthread case).

> +
> +             int nlwps = p->p_nlwps;
> +             (void)chglwpcnt(kauth_cred_getuid(ncred), -nlwps);
> +             (void)chglwpcnt(r, nlwps);
> +

You need to acquire p->p_lock around this.  I suppose this is the
"atomicity issue" mentioned above. :)

> @@ -805,6 +807,7 @@ lwp_exit_switchaway(lwp_t *l)
>       LOCKDEBUG_BARRIER(NULL, 0);
>  
>       kstack_check_magic(l);
> +     chglwpcnt(kauth_cred_geteuid(l->l_cred), -1);
>  

Since this was done in lwp_exit(), why again?

> +#define _MEM(n) { # n, offsetof(struct uidinfo, ui_ ## n) }
> +             _MEM(proccnt),
> +             _MEM(lwpcnt),
> +             _MEM(lockcnt),
> +             _MEM(sbsize),
> +#undef _MEM

I would avoid macro, it is only four entries. :)

> +                     node.sysctl_data = &cnt;
> +                     uip = uid_find(kauth_cred_geteuid(l->l_cred));
> +
> +                     *(uint64_t *)node.sysctl_data = 
> +                         *(u_long *)((char *)uip + nv[i].value);

Potentially unaligned access?  Use memcpy()?

> Index: sys/sysctl.h
> ===================================================================
> RCS file: /cvsroot/src/sys/sys/sysctl.h,v
> retrieving revision 1.199
> diff -u -p -u -r1.199 sysctl.h
> --- sys/sysctl.h      27 Jan 2012 19:48:41 -0000      1.199
> +++ sys/sysctl.h      23 May 2012 23:19:36 -0000
> @@ -266,7 +266,8 @@ struct ctlname {
>  #define      KERN_SYSVIPC            82      /* node: SysV IPC
>  #parameters */ define        KERN_BOOTTIME           83      /*
>  #struct: time kernel was booted */ define
>  #KERN_EVCNT          84      /* struct: evcnts */
> -#define      KERN_MAXID              85      /* number of valid
> kern ids */ +#define  KERN_MAXLWP             85      /* int:
> maxlwp */ +#define    KERN_MAXID              86      /* number
> of valid kern ids */ 

Use CTL_CREATE instead of static entry (KERN_MAXLWP)?  I thought we are
trying to convert everything to dynamic creation/lookup and not add more
hardcoded sysctl nodes.

-- 
Mindaugas