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;
>   }
>
>
>