Re: [ros-dev] [ros-diffs] [mnordell] 29826: First small attempt at implementing process memory quota. Currently disabled without explicit code modification (enabled by macro) to not modify behaviour of trunk.

[Alex Ionescu]

Couple of comments:
1) Why implement new, complex features in the kernel instead of
fixing current bugs? I know it's fun to start hacking on new code,
but priorities should be to implement stuff that's required not to
break other apps/add functionality, not internal features which will
take ages to implement (the Ps functions for quota are "easy", it's
the Mm part that's bad).
I had already written all these functions, but deleted them, because
nobody needed them, and the Mm part was too complex to do.
2) Why are you bothering with PspPoolQuotaIndexFromPoolType? In case
you haven't noticed, there's something called pool masks which make
the operation very easy without requiring a function call (such as
POOL_TYPE_MASK or BASE_POOL_MASK, forgot how it's called).
3) INT is not an acceptable kernel type.
4) We don't use "static" in the kernel.
5) Instead of duplicating code, perhaps consider calling a main
worker function such as PspChargeQuota?
6) Consider using an enum instead of magic numbers inside the quota
entries, I believe it's called PS_QUOTA_TYPE.
7) The quota routines require interlocks, otherwise they will not be
thread safe.
8) They also require a global spinlock in the "give back" case.
9) Committing code that won't even build, even with the define on,
isn't very appropriate. Trunk isn't a playground. If someone wants to
actually test this, they won't be able to build it because of things
like "refuse". SVN should contain code that builds at any time. It's
not hard to write /* TODO */..
Just my 2 cents
--
Best regards,
Alex Ionescu
On 23-Oct-07, at 6:05 AM, mnordell(a)svn.reactos.org wrote:
>   Author: mnordell
>  Date: Tue Oct 23 14:05:40 2007
>  New Revision: 29826
>  URL: http://svn.reactos.org/svn/reactos?rev=29826&view=rev
>  Log:
>  First small attempt at implementing process memory quota. Currently
>  disabled without explicit code modification (enabled by macro) to
>  not modify behaviour of trunk.
>  Modified:
>      trunk/reactos/ntoskrnl/ps/quota.c
>  Modified: trunk/reactos/ntoskrnl/ps/quota.c
>  URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ps/
>  quota.c?rev=29826&r1=29825&r2=29826&view=diff
>  ======================================================================
>  ========
>  --- trunk/reactos/ntoskrnl/ps/quota.c (original)
>  +++ trunk/reactos/ntoskrnl/ps/quota.c Tue Oct 23 14:05:40 2007
>  @@ -15,6 +15,12 @@
>   EPROCESS_QUOTA_BLOCK PspDefaultQuotaBlock;
>  +
>  +/* Define this macro to enable quota code testing. Once quota code
>  is */
>  +/* stable and verified, remove this macro and checks for it. */
>  +/*#define PS_QUOTA_ENABLE_QUOTA_CODE*/
>  +
>  +
>   /* FUNCTIONS
>  ***************************************************************/
>   VOID
>  @@ -66,9 +72,29 @@
>   {
>       /* Don't do anything for the system process */
>       if (Process == PsInitialSystemProcess) return STATUS_SUCCESS;
>  -
>  +
>  +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
>  +    if (Process)
>  +    {
>  +        /* TODO: Check with Process->QuotaBlock if this can be
>  satisfied, */
>  +        /* assuming this indeed is the place to check it. */
>  +        /* Probably something like:
>  +        if (Process->QuotaUsage[2] + Amount >
>  +            Process->QuotaBlock->QuotaEntry[2].Limit)
>  +        {
>  +            refuse
>  +        }
>  +        */
>  +        Process->QuotaUsage[2] += Amount;
>  +        if (Process->QuotaPeak[2] < Process->QuotaUsage[2])
>  +        {
>  +            Process->QuotaPeak[2] = Process->QuotaUsage[2];
>  +        }
>  +    }
>  +#else
>       /* Otherwise, not implemented */
>       UNIMPLEMENTED;
>  +#endif
>       return STATUS_SUCCESS;
>   }
>  @@ -115,6 +141,38 @@
>       return PsChargeProcessPoolQuota(Process, PagedPool, Amount);
>   }
>  +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
>  +/*
>  + * Internal helper function.
>  + * Returns the index of the Quota* member in EPROCESS for
>  + * a specified pool type, or -1 on failure.
>  + */
>  +static
>  +INT
>  +PspPoolQuotaIndexFromPoolType(POOL_TYPE PoolType)
>  +{
>  +    switch (PoolType)
>  +    {
>  +        case NonPagedPool:
>  +        case NonPagedPoolMustSucceed:
>  +        case NonPagedPoolCacheAligned:
>  +        case NonPagedPoolCacheAlignedMustS:
>  +        case NonPagedPoolSession:
>  +        case NonPagedPoolMustSucceedSession:
>  +        case NonPagedPoolCacheAlignedSession:
>  +        case NonPagedPoolCacheAlignedMustSSession:
>  +            return 1;
>  +        case PagedPool:
>  +        case PagedPoolCacheAligned:
>  +        case PagedPoolSession:
>  +        case PagedPoolCacheAlignedSession:
>  +            return 0;
>  +        default:
>  +            return -1;
>  +    }
>  +}
>  +#endif
>  +
>   /*
>    * @implemented
>    */
>  @@ -124,10 +182,32 @@
>                            IN POOL_TYPE PoolType,
>                            IN ULONG Amount)
>   {
>  +    INT PoolIndex;
>       /* Don't do anything for the system process */
>       if (Process == PsInitialSystemProcess) return STATUS_SUCCESS;
>  -    UNIMPLEMENTED;
>  +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
>  +    PoolIndex = PspPoolQuotaIndexFromPoolType(PoolType);
>  +    if (Process && PoolIndex != -1)
>  +    {
>  +        /* TODO: Check with Process->QuotaBlock if this can be
>  satisfied, */
>  +        /* assuming this indeed is the place to check it. */
>  +        /* Probably something like:
>  +        if (Process->QuotaUsage[PoolIndex] + Amount >
>  +            Process->QuotaBlock->QuotaEntry[PoolIndex].Limit)
>  +        {
>  +            refuse
>  +        }
>  +        */
>  +        Process->QuotaUsage[PoolIndex] += Amount;
>  +        if (Process->QuotaPeak[PoolIndex] < Process->QuotaUsage
>  [PoolIndex])
>  +        {
>  +            Process->QuotaPeak[PoolIndex] = Process->QuotaUsage
>  [PoolIndex];
>  +        }
>  +    }
>  +#else
>  +    UNIMPLEMENTED;
>  +#endif
>       return STATUS_SUCCESS;
>   }
>  @@ -140,10 +220,26 @@
>                     IN POOL_TYPE PoolType,
>                     IN ULONG_PTR Amount)
>   {
>  +    INT PoolIndex;
>       /* Don't do anything for the system process */
>       if (Process == PsInitialSystemProcess) return;
>  -    UNIMPLEMENTED;
>  +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
>  +    PoolIndex = PspPoolQuotaIndexFromPoolType(PoolType);
>  +    if (Process && PoolIndex != -1)
>  +    {
>  +        if (Process->QuotaUsage[PoolIndex] < Amount)
>  +        {
>  +            DPRINT1("WARNING: Process->QuotaUsage sanity check
>  failed.\n");
>  +        }
>  +        else
>  +        {
>  +            Process->QuotaUsage[PoolIndex] -= Amount;
>  +        }
>  +    }
>  +#else
>  +    UNIMPLEMENTED;
>  +#endif
>   }
>   /*
>  @@ -157,7 +253,11 @@
>       /* Don't do anything for the system process */
>       if (Process == PsInitialSystemProcess) return;
>  -    UNIMPLEMENTED;
>  +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
>  +    PsReturnPoolQuota(Process, NonPagedPool, Amount);
>  +#else
>  +    UNIMPLEMENTED;
>  +#endif
>   }
>   /*
>  @@ -171,7 +271,11 @@
>       /* Don't do anything for the system process */
>       if (Process == PsInitialSystemProcess) return;
>  -    UNIMPLEMENTED;
>  +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
>  +    PsReturnPoolQuota(Process, PagedPool, Amount);
>  +#else
>  +    UNIMPLEMENTED;
>  +#endif
>   }
>   NTSTATUS
>  @@ -182,8 +286,22 @@
>       /* Don't do anything for the system process */
>       if (Process == PsInitialSystemProcess) return STATUS_SUCCESS;
>  +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
>  +    if (Process)
>  +    {
>  +        if (Process->QuotaUsage[2] < Amount)
>  +        {
>  +            DPRINT1("WARNING: Process PageFileQuotaUsage sanity
>  check failed.\n");
>  +        }
>  +        else
>  +        {
>  +            Process->QuotaUsage[2] -= Amount;
>  +        }
>  +    }
>  +#else
>       /* Otherwise, not implemented */
>       UNIMPLEMENTED;
>  +#endif
>       return STATUS_SUCCESS;
>   }
>