Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch per-numa memory usage from the OS. #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kilobyte
Copy link
Collaborator

@kilobyte kilobyte commented Nov 17, 2021

This change is Reviewable

Copy link
Owner

@bratpiorka bratpiorka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not strictly related to hot poc - could you extend print_memtier_memory() with this info?

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kilobyte)

src/numus.c Outdated
#include <stdint.h>
#include <stdlib.h>

#define MAX_NODES 1024 /* kernel max */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be a better idea to use CONFIG_NODES_SHIFT or some other #def ?
What about this code?

src/numus.c Outdated
}

if (maxnode && !pagesize)
return fprintf(stderr, "Pages used but no page size\n"), false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return without fclose(f)

src/numus.c Outdated
Comment on lines 48 to 60
else if (tok[0] == 'N' && tok[1] >= '0' && tok[1] <= '9') {
int node = atoi(tok + 1);
if (node >= MAX_NODES)
continue;
if (maxnode + 1 >= MAX_NODES)
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments would be welcome, the code could be more readable

if (!valp) {
/* flags */
if (!strcmp(tok, "stack"))
goto skip_mapping;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't simple '''break''' have the same effect?

Copy link
Collaborator Author

@kilobyte kilobyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The values are global, not sure if it's good to print them there. On the other hand, if we marked that as global...

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @maciekpac and @PatKamin)


src/numus.c, line 7 at r1 (raw file):

Previously, maciekpac wrote…

Maybe it would be a better idea to use CONFIG_NODES_SHIFT or some other #def ?
What about this code?

CONFIG_NODES_SHIFT is a settable piece of kernel config, and thus can't be used outside of the kernel itself. I don't see a #define for the max in the kernel's uapi either.


src/numus.c, line 41 at r1 (raw file):

Previously, maciekpac wrote…

wouldn't simple '''break''' have the same effect?

Alas, we're in a deeper while.


src/numus.c, line 53 at r1 (raw file):

Previously, maciekpac wrote…

some comments would be welcome, the code could be more readable

I've added some.


src/numus.c, line 61 at r1 (raw file):

Previously, maciekpac wrote…

return without fclose(f)

Done.

Copy link
Owner

@bratpiorka bratpiorka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok but otherwise this code wouldn't be used and tested at all

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @maciekpac and @PatKamin)

Copy link
Collaborator

@PatKamin PatKamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kilobyte and @maciekpac)


src/numus.c, line 7 at r2 (raw file):

#include <stdlib.h>

#define MAX_NODES 1024 /* kernel max */

Perhaps you could use numa_num_possible_nodes() from numa library.


src/numus.c, line 75 at r2 (raw file):

        }
        for (int i = 0; i < maxnode; i++)
            numamem[num[i]].used += count[i] * pagesize;

size_t += long * unsigned long long

Copy link
Collaborator Author

@kilobyte kilobyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @bratpiorka, @maciekpac, and @PatKamin)


src/numus.c, line 7 at r2 (raw file):

Previously, PatKamin (Patryk Kaminski) wrote…

Perhaps you could use numa_num_possible_nodes() from numa library.

In this version, the struct is static thus I'm using a compile-time value (that matches kernel max) — but once we get to polishing this code for outside usage, that's exactly the function we should use. Thanks for researching this bit, I've put it in a comment so it's not forgotten.


src/numus.c, line 75 at r2 (raw file):

Previously, PatKamin (Patryk Kaminski) wrote…

size_t += long * unsigned long long

count[i] is size_t, pagesize is uint64_t, both unsigned. Am I missing something?
I've just changed pagesize to size_t for consistency, but that's the same effective type.

Copy link
Collaborator

@PatKamin PatKamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kilobyte and @maciekpac)


src/numus.c, line 75 at r2 (raw file):

Previously, kilobyte (Adam Borowski) wrote…

count[i] is size_t, pagesize is uint64_t, both unsigned. Am I missing something?
I've just changed pagesize to size_t for consistency, but that's the same effective type.

Indeed. I wrongly assumed that count[i] is long because you use atol() when assigning it's value.


src/numus.c, line 62 at r3 (raw file):

                    continue;
                num[maxnode] = node;
                count[maxnode] = atol(valp);

atol() return long whereas count[maxnode] is an ull.

Copy link
Collaborator

@maciekpac maciekpac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kilobyte and @PatKamin)


src/numus.c, line 62 at r3 (raw file):

Previously, PatKamin (Patryk Kaminski) wrote…

atol() return long whereas count[maxnode] is an ull.

up

Copy link
Collaborator Author

@kilobyte kilobyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @PatKamin)


src/numus.c, line 75 at r2 (raw file):

Previously, PatKamin (Patryk Kaminski) wrote…

Indeed. I wrongly assumed that count[i] is long because you use atol() when assigning it's value.

strtoul()


src/numus.c, line 62 at r3 (raw file):

Previously, maciekpac wrote…

up

Changed to unchecked strtoul().

Copy link
Collaborator

@PatKamin PatKamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kilobyte)

@kilobyte
Copy link
Collaborator Author

kilobyte commented Dec 2, 2021

Rebased.

Copy link
Collaborator

@maciekpac maciekpac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kilobyte)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants