Re: [LTP] [PATCH v2] mmap/mmap12.c: new case to test MAP_POPULATE of mmap

Re: [LTP] [PATCH v2] mmap/mmap12.c: new case to test MAP_POPULATE of mmap

[chrubis]

Hi!
> +static int page_check(void)
> +{
> +	int pid;
> +	int ret;
> +	int pm;
> +	int num_pages;
> +	long index;
> +	off_t offset;
> +	size_t page_sz;
> +	char maps_dev[PATHLEN];
> +	char pagemap_dev[PATHLEN];
> +	char line[LINELEN];
> +	FILE *maps;
> +	unsigned long vm_start;
> +	unsigned long vm_end;
> +	unsigned long long pagemap;
> +
> +	page_sz = getpagesize();
> +
> +	pid = getpid();
> +	sprintf(maps_dev, "/proc/%d/maps", pid);
> +	sprintf(pagemap_dev, "/proc/%d/pagemap", pid);

Just use "/proc/self/maps" and "/proc/self/pagemap" instead.

> +	maps = fopen(maps_dev, "r");
> +	if (maps == NULL)
> +		tst_brkm(TFAIL | TERRNO, NULL, "Open dev maps failed");
> +
> +	while (fgets(line, LINELEN, maps) != NULL)
> +		if (strstr(line, TEMPFILE) != NULL)
> +			break;
> +
> +	ret = sscanf(line, "%lX-%lX", &vm_start, &vm_end);
> +	if (ret != 2)
> +		tst_brkm(TFAIL | TERRNO, NULL, "Get address space failed");

This part of the code does not cope with unlikely condition that
TEMPFILE is missing from /proc/self/maps we should fail the test in this
case with proper error message.

> +	num_pages = (vm_end - vm_start) / page_sz;
> +	index = (vm_start / page_sz) * sizeof(unsigned long long);
> +
> +	pm = open(pagemap_dev, O_RDONLY);
> +	if (pm == -1)
> +		tst_brkm(TFAIL | TERRNO, NULL, "Open dev pagemap failed");
> +
> +	offset = lseek(pm, index, SEEK_SET);
> +	if (offset != index)
> +		tst_brkm(TFAIL | TERRNO, NULL, "Reposition offset failed");
> +
> +	while (num_pages > 0) {
> +		ret = read(pm, &pagemap, sizeof(unsigned long long));
> +		if (ret < 0)
> +			tst_brkm(TFAIL | TERRNO, NULL, "Read pagemap failed");
> +		/*
> +		 * Check if the page is present.
> +		 */
> +		if (!(pagemap & (1ULL<<63))) {
> +			close(pm);
> +			fclose(maps);
> +			return 1;

I would perpahs be more verbose here and go over all of the pages and
list these that are not mapped, but that is a minor nitpick. 

> +		}
> +		num_pages--;
> +	}
> +
> +	close(pm);
> +	fclose(maps);
> +
> +	return 0;
> +}

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH v2] mmap/mmap12.c: new case to test MAP_POPULATE of mmap

[chrubis]

> >> +	while (num_pages > 0) {
> >> +		ret = read(pm, &pagemap, sizeof(unsigned long long));
> >> +		if (ret < 0)
> >> +			tst_brkm(TFAIL | TERRNO, NULL, "Read pagemap failed");
> >> +		/*
> >> +		 * Check if the page is present.
> >> +		 */
> >> +		if (!(pagemap & (1ULL<<63))) {
> >> +			close(pm);
> >> +			fclose(maps);
> >> +			return 1;
> > 
> > I would perpahs be more verbose here and go over all of the pages and
> > list these that are not mapped, but that is a minor nitpick. 
> > 
> 
> You mean listing every page which is not present?

Yes please, nothing fancy just short message with address of the page
and its ordering number in the mapping printed using tst_resm with
TINFO.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH v2] mmap/mmap12.c: new case to test MAP_POPULATE of mmap

[DAN LI]

On 06/25/2013 06:47 PM, Jan Stancek wrote:
> 
> ----- Original Message -----
>> From: "DAN LI" <li.dan@cn.fujitsu.com>
>> To: "LTP list" <ltp-list@lists.sourceforge.net>
>> Cc: "Jan Stancek" <jstancek@redhat.com>, chrubis@suse.cz, "Caspar Zhang" <caspar@casparzhang.com>
>> Sent: Tuesday, 25 June, 2013 11:13:27 AM
>> Subject: [LTP] [PATCH v2] mmap/mmap12.c: new case to test MAP_POPULATE of mmap
>>
>>
>> Create test case for MAP_POPULATE of mmap.
>>
>> Verify the following statement
>> "For a file mapping,this causes read-ahead on the file.
>>  Later accesses to the mapping will not be blocked by page faults."
>> by mmap-ing a file and check if all pages are present.
>>
>>
>> Signed-off-by: DAN LI <li.dan@cn.fujitsu.com>
>> ---
>>  runtest/syscalls                        |   1 +
>>  testcases/kernel/syscalls/mmap/mmap12.c | 194
>>  ++++++++++++++++++++++++++++++++
>>  2 files changed, 195 insertions(+)
>>  create mode 100644 testcases/kernel/syscalls/mmap/mmap12.c
> 
> <snip>
> 
>> +
>> +static int page_check(void)
>> +{
>> +	int pid;
>> +	int ret;
>> +	int pm;
>> +	int num_pages;
>> +	long index;
>> +	off_t offset;
>> +	size_t page_sz;
>> +	char maps_dev[PATHLEN];
>> +	char pagemap_dev[PATHLEN];
>> +	char line[LINELEN];
>> +	FILE *maps;
>> +	unsigned long vm_start;
>> +	unsigned long vm_end;
>> +	unsigned long long pagemap;
>> +
>> +	page_sz = getpagesize();
>> +
>> +	pid = getpid();
>> +	sprintf(maps_dev, "/proc/%d/maps", pid);
>> +	sprintf(pagemap_dev, "/proc/%d/pagemap", pid);
>> +
>> +	maps = fopen(maps_dev, "r");
>> +	if (maps == NULL)
>> +		tst_brkm(TFAIL | TERRNO, NULL, "Open dev maps failed");
>> +
>> +	while (fgets(line, LINELEN, maps) != NULL)
>> +		if (strstr(line, TEMPFILE) != NULL)
>> +			break;
>> +
>> +	ret = sscanf(line, "%lX-%lX", &vm_start, &vm_end);
> 
> Why parsing these 2 from maps? Size is defined by you, and you get vm_start
> from mmap().

Just a force of inertia following pagemap document.

> 
>> +	if (ret != 2)
>> +		tst_brkm(TFAIL | TERRNO, NULL, "Get address space failed");
>> +
>> +	num_pages = (vm_end - vm_start) / page_sz;
>> +	index = (vm_start / page_sz) * sizeof(unsigned long long);
> 
> C99 says long long is _at least_ 64 bits wide. If it's larger than 64, formula
> above would break. How about using type, that's exactly 64 bit (uint64_t)?

Get it.

> 
>> +
>> +	pm = open(pagemap_dev, O_RDONLY);
>> +	if (pm == -1)
>> +		tst_brkm(TFAIL | TERRNO, NULL, "Open dev pagemap failed");
>> +
>> +	offset = lseek(pm, index, SEEK_SET);
>> +	if (offset != index)
>> +		tst_brkm(TFAIL | TERRNO, NULL, "Reposition offset failed");
>> +
>> +	while (num_pages > 0) {
>> +		ret = read(pm, &pagemap, sizeof(unsigned long long));
>> +		if (ret < 0)
>> +			tst_brkm(TFAIL | TERRNO, NULL, "Read pagemap failed");
>> +		/*
>> +		 * Check if the page is present.
>> +		 */
>> +		if (!(pagemap & (1ULL<<63))) {
>> +			close(pm);
>> +			fclose(maps);
>> +			return 1;
>> +		}
>> +		num_pages--;
>> +	}
>> +
>> +	close(pm);
>> +	fclose(maps);
>> +
>> +	return 0;
>> +}
>> +
>> +static void setup(void)
>> +{
>> +	tst_sig(FORK, DEF_HANDLER, cleanup);
>> +
>> +	if ((tst_kvercmp(2, 6, 25)) < 0)
>> +		tst_brkm(TCONF, NULL,
>> +			"This test can only run on kernels that are 2.6.23 and "
> 
> Message says 2.6.23, but check is for 2.6.25.

Wow...


All of the above will be fixed in coming V3.


Thank you for reviewing.

Regards,
DAN LI

> 
>> +			"higher");
>> +
>> +	TEST_PAUSE;
>> +
>> +	tst_tmpdir();
>> +
>> +	fildes = open(TEMPFILE, O_RDWR | O_CREAT, 0766);
>> +	if (fildes < 0)
>> +		tst_brkm(TFAIL, cleanup, "opening %s failed", TEMPFILE);
>> +
>> +	if (ftruncate(fildes, MMAPSIZE) < 0)
>> +		tst_brkm(TFAIL | TERRNO, cleanup, "ftruncate file failed");
>> +
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	close(fildes);
>> +	TEST_CLEANUP;
>> +	tst_rmdir();
>> +}
>> --
>> 1.8.1
>>
> 



------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list