Re: [LTP] [PATCH] make pid_list dynamically sized with memory

[Garrett C. <yan...@gm...>]

In addition to what Hannu mentioned...

1. It seems silly that the test is split in to s390 / 32-bit / 64-bit when all of this stuff is limit driven AFAICS. Would be nice if this was all folded in one body of test code with a minimal amount of setup code.
2. More comments inline.

On Dec 22, 2010, at 11:02 PM, Caspar Zhang <cz...@re...> wrote:

> ping... any comments?
> 
> On 12/21/2010 06:12 PM, Caspar Zhang wrote:
>> We get segfaults during testing mtest01 on a 5TB memory machine, the
>> problem was traced to the array pdi_list[] went to overflow and
>> corrupted memory. This fix makes pid_list[] dynamically sized with
>> correct memory size to avoid overflow.
>> 
>> Signed-off-by: Caspar Zhang<cz...@re...>
>> ---
>>  testcases/kernel/mem/mtest01/mtest01.c |   50 +++++++++++++++++++------------
>>  1 files changed, 31 insertions(+), 19 deletions(-)
>> 
>> diff --git a/testcases/kernel/mem/mtest01/mtest01.c b/testcases/kernel/mem/mtest01/mtest01.c
>> index e4e9a56..1362e08 100644
>> --- a/testcases/kernel/mem/mtest01/mtest01.c
>> +++ b/testcases/kernel/mem/mtest01/mtest01.c
>> @@ -44,6 +44,10 @@
>> 
>>  #include "test.h"
>> 
>> +#define MAX_31BIT_SIZE (unsigned long)(500*1024*1024)
>> +#define MAX_32BIT_SIZE (unsigned long)(1024*1024*1024)
>> +#define MAX_64BIT_SIZE (unsigned long long)(3*1024*1024*1024)
>> +
>>  char *TCID = "mtest01";
>>  int TST_TOTAL = 1;
>> 
>> @@ -62,11 +66,12 @@ int main(int argc, char* argv[]) {
>>    unsigned long bytecount, alloc_bytes;
>>    unsigned long long original_maxbytes,maxbytes=0;
>>    unsigned long long pre_mem, post_mem;
>> +  unsigned long long total_ram, total_free, D, C;
>>    extern char* optarg;
>>    int chunksize = 1024*1024; /* one meg at a time by default */
>>    struct sysinfo sstats;
>>    int i,pid_cntr;
>> -  pid_t pid,pid_list[1000];
>> +  pid_t pid,*pid_list;
>>    struct sigaction act;
>> 
>>    act.sa_handler = handler;
>> @@ -74,8 +79,18 @@ int main(int argc, char* argv[]) {
>>    sigemptyset(&act.sa_mask);
>>    sigaction(SIGRTMIN,&act, 0);
>> 
>> -  for (i=0;i<1000;i++)
>> -   pid_list[i]=(pid_t)0;
>> +  sysinfo(&sstats);
>> +  total_ram=sstats.totalram;
>> +  total_ram=total_ram+sstats.totalswap;
>> +#if defined (_s390_)
>> +  pidlist_sz = total_ram / MAX_31BIT_SIZE + 1;
>> +#elif __WORDSIZE==32
>> +  pidlist_sz = total_ram / MAX_32BIT_SIZE + 1;
>> +#elif __WORDSIZE==64
>> +  pidlist_sz = total_ram / MAX_64BIT_SIZE + 1;
>> +#endif
>> +  pid_list = (pid_t *)malloc(sizeof(pid_t) * pidlist_sz);

You aren't checking the return from malloc and the cast is unnecessary.

>> +  memset(pid_list, 0, sizeof(pid_t) * pidlist_sz);
>> 
>>    while ((c=getopt(argc, argv, "c:b:p:wvh")) != EOF) {
>>      switch((char)c) {
>> @@ -115,12 +130,8 @@ int main(int argc, char* argv[]) {
>> 
>>    sysinfo(&sstats);
>>    if (maxpercent) {
>> -    unsigned long long total_ram, total_free, D, C;
>>      percent=(float)maxpercent/100.00;
>> 
>> -    total_ram=sstats.totalram;
>> -    total_ram=total_ram+sstats.totalswap;
>> -
>>      total_free=sstats.freeram;
>>      total_free=total_free+sstats.freeswap;
>> 
>> @@ -157,47 +168,47 @@ int main(int argc, char* argv[]) {
>>      pid_list[i]=pid;
>> 
>>  #if defined (_s390_) /* s390's 31bit addressing requires smaller chunks */
>> -  while ( (pid!=0)&&  (maxbytes>  500*1024*1024) )
>> +  while ((pid!=0)&&  (maxbytes>  MAX_31BIT_SIZE))

If you're going to trim the whitespace, please remove the unnecessary parentheses and add whitespace around the logic operators please.

>>    {
>>      i++;
>> -    maxbytes=maxbytes-(500*1024*1024);
>> +    maxbytes -= MAX_31BIT_SIZE;
>>      pid=fork();
>>      if (pid != 0)
>>        pid_cntr++;
>>        pid_list[i]=pid;
>>    }
>> -  if ( maxbytes>  500*1024*1024 )
>> -    alloc_bytes=500*1024*1024;
>> +  if (maxbytes>  MAX_31BIT_SIZE)
>> +    alloc_bytes = MAX_31BIT_SIZE;
>>    else
>>      alloc_bytes=(unsigned long)maxbytes;
>> 
>>  #elif __WORDSIZE==32
>> -  while ( (pid!=0)&&  (maxbytes>  1024*1024*1024) )
>> +  while ((pid!=0)&&  (maxbytes>  MAX_32BIT_SIZE))
>>    {
>>      i++;
>> -    maxbytes=maxbytes-(1024*1024*1024);
>> +    maxbytes -= MAX_32BIT_SIZE;
>>      pid=fork();
>>      if (pid != 0)
>>        pid_cntr++;
>>        pid_list[i]=pid;
>>    }
>> -  if ( maxbytes>  1024*1024*1024 )
>> -    alloc_bytes=1024*1024*1024;
>> +  if (maxbytes>  MAX_32BIT_SIZE)
>> +    alloc_bytes = MAX_32BIT_SIZE;
>>    else
>>      alloc_bytes=(unsigned long)maxbytes;
>> 
>>  #elif __WORDSIZE==64
>> -  while ( (pid!=0)&&  (maxbytes>  (unsigned long long)3*1024*1024*1024) )
>> +  while ((pid!=0)&&  (maxbytes>  MAX_64BIT_SIZE))
>>    {
>>      i++;
>> -    maxbytes=maxbytes-(unsigned long long)3*1024*1024*1024;
>> +    maxbytes - MAX_64BIT_SIZE;
>>      pid=fork();
>>      if (pid != 0)
>>        pid_cntr++;
>>        pid_list[i]=pid;
>>    }
>> -  if ( maxbytes>  (unsigned long long)3*1024*1024*1024 )
>> -    alloc_bytes=(unsigned long long)3*1024*1024*1024;
>> +  if (maxbytes>  MAX_64BIT_SIZE)
>> +    alloc_bytes = MAX_64BIT_SIZE;
>>    else
>>      alloc_bytes=(unsigned long)maxbytes;
>>  #endif
>> @@ -258,5 +269,6 @@ int main(int argc, char* argv[]) {
>>      else
>>        tst_resm(TPASS, "%llu kbytes allocated only.", original_maxbytes/1024);
>>    }
>> +  free(pid_list);
>>    exit(0);
>>  }