Re: [LTP] [RFC][PATCH 0/2] Add safe_get_nodemap()

[Petr Vorel <pvorel@suse.cz>]

> Hi!
> > not a huge improvement, but because all tst_get_nodemap() are in the
> > setup (and only ksm06.c allows input parameter) we could have struct
> > tst_test member which would call safe_get_nodemap().

> > e.g.:

> > .nodemap = (const struct tst_path_val) {
> > 		.type = TST_NUMA_MEM
> > 		.required = 2,
> > },

> I do not get this, the struct tst_path_val is something completely
> different.

I'm sorry, copy paste error. This was meant to be a new struct,
e.g. struct tst_nodemap_val. But I see you below suggested struct tst_numa.

> > safe_get_nodemap(tst_test->nodemap->type,
> > 	tst_test->nodemap->required * getpagesize() / 1024);

> > This would not work for non - page sized nodes, e.g.:
> > nodes = tst_get_nodemap(TST_NUMA_MEM, 2 * PAGES_ALLOCATED * page_size / 1024);

> > => extra member would need to be added:

> > .nodemap = (const struct tst_path_val) {
> > 		.type = TST_NUMA_MEM
> > 		.required = 2,
> > 		.size = PAGES_ALLOCATED, // default == 1
> > },

> I've avoided the size to be in pages and choosen kilobytes instead
> because the page size can differ a lot.

Make sense.

> > would call:

> > safe_get_nodemap(tst_test->nodemap->type,
> > 	tst_test->nodemap->required * tst_test->nodemap->size * getpagesize() / 1024,
>          ^
> 	 This does not make any sense.

> The memory parameter is supposed to be minimal free memory on each node,
> for each node we check that it has at least min_mem_kb in the
> node_has_enough_memory() function.

> There is no point in multiplying that by the number of nodes we require.

> And this looks like it uses kilobytes not pages.

> > 	tst_test->nodemap->required);

> I guess that what you meant is that we will add tst_numa structure such as:

> struct tst_numa {
> 	enum tst_numa_types node_type;
> 	unsigned int min_nodes;
> 	unsigned int min_mem_kb;
> };

+1, thanks for suggesting a proper struct :).

BTW why we can use enum tst_numa_types in struct and not as function parameter?

-struct tst_nodemap *tst_get_nodemap(int type, size_t min_mem_kb);
+struct tst_nodemap *tst_get_nodemap(enum tst_numa_types type, size_t min_mem_kb);

Old toolchains? Maybe not needed nowadays? I guess with using enum we could get
rid of the check for enum validity:
if (type & ~(TST_NUMA_MEM))
	tst_brk(TBROK, "Invalid type %i\n", type);

> And then use that in tst_test structure. That would certainly make
> sense, but I guess that we would have to move the numa library to the
> lib/ as well. I'm not sure that we can have a function call from
> tst_test.c library to something that is not compiled in by default.

Correct, file is in libs/numa/tst_numa.c. But how about use predecessor check?

if (tst_test->numa) {
#ifdef HAVE_NUMA_V2
	...
#else
	tst_brk(TCONF, NUMA_ERROR_MSG);
#endif
}

With help we would even get rid of else part of the compile check in the
tests:

#else
TST_TEST_TCONF(NUMA_ERROR_MSG);

But we would have modify lib/Makefile to conditionally add "-lnuma -lltpnuma".
"-lnuma" would be based on NUMA_LIBS (stored in include/mk/config.mk, detected
in m4/ltp-numa.m4), e.g. something like this could work:

ifneq ($(NUMA_LIBS),)
LDLIBS += $(NUMA_LIBS) -lltpnuma
endif

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp