Re: [RFC PATCH v4 4/4] selftests/sgx: Add test for madvise(..., WILLNEED)

["Haitao Huang" <haitao.huang@linux.intel.com>]

On Tue, 14 Feb 2023 20:38:29 -0600, Huang, Kai <kai.huang@intel.com> wrote:

> On Fri, 2023-01-27 at 20:55 -0800, Haitao Huang wrote:
>> +/*
>> + * Compare performance with and without madvise call before EACCEPT'ing
>> + * different size of regions.
>> + */
>> +TEST_F_TIMEOUT(enclave, augment_via_madvise, TIMEOUT_DEFAULT)
>> +{
>>
> [...]
>
>> +
>> +	for (i = 0; i < self->encl.nr_segments; i++) {
>> +		struct encl_segment *seg = &self->encl.segment_tbl[i];
>> +
>> +		total_size += seg->size;
>> +	}
>> +
>> +	for (i = 1; i < 52 && advise_size < max_advise_size; i++) {
>> +		addr = mmap((void *)self->encl.encl_base + total_size, advise_size,
>> +			    PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
>> +					self->encl.fd, 0);
>
> I see the problem now.  Here 'pgoff' is always 0.  I think this is wrong.
>
> Shouldn't you use the actual offset relative to the file as pgoff, which  
> is
> 	
> 	total_size >> PAGE_SHIFT
>
> ?
>
But that will be inconsistent with current usage. We have been using zero  
offset always including these self tests. The offset is also redundant in  
this case because it is totally defined by the address for a given enclave  
fd.

>
> [...]
>
>> +	    munmap(addr, advise_size);
>
> Incorrect indent here?
>
> Perhaps you should use checkpatch.pl?
>

hmm, I did run it:
  scripts/checkpatch.pl --strict  
advise_patch_RFC_v4/v4-0004-selftests-sgx-Add-test-for-madvise-.-WILLNEED.patch
total: 0 errors, 0 warnings, 0 checks, 179 lines checked

Will fix.

Thanks
Haitao