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

Haitao Huang
2023-02-15T04:43:04Z

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