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

libnuma: introduce an API to outdate cpu to node mapping #73

Merged
merged 1 commit into from
Sep 16, 2019

Conversation

pfliu
Copy link

@pfliu pfliu commented Jul 23, 2019

numa_node_to_cpus() caches the cpu to node mapping, and not updates it
during the cpu online/offline event.

Ideally, in order to update the mapping automatically, it requires
something like udev to spy on kernel event socket, and update cache if
event happen. This solution is a little complicated inside a libnuma.so. In
stead of doing so, exposing an API numa_node_to_cpu_outdated() for user,
and saddling the event-detecting task to the user.

So the user of libnuma can work using either of the following models:
-1. blindless outdate cache if careless about performance
numa_node_to_cpu_outdated();
numa_node_to_cpu();
-2. event driven spy on kernel event, if happened, call
numa_node_to_cpu_outdated();

Signed-off-by: Pingfan Liu piliu@redhat.com

@pfliu
Copy link
Author

pfliu commented Jul 23, 2019

There was a try to remove cache data: https://www.spinics.net/lists/linux-numa/msg01134.html
But I think my implement can totally avoid the performance drop if no cpu online/offline event happen

@pfliu
Copy link
Author

pfliu commented Jul 29, 2019

Could anybody help to review it?
Thanks,
Pingfan

@andikleen
Copy link
Contributor

Sorry for the delay.

I think we need a mechanism to free the old mask eventually. Otherwise if some application calls this API frequently there might be a unlimited long term memory leak. But I don't see a way to free it. The only reasonable way I can think of is to add a list and reuse masks with the same value. That would guarantee that any leak is bounded at least to all possible CPU masks. Drawback is that it will require locking, so will add more dependencies to libnuma.

I don't like the term "outdated". Can you just use "update" ?

The new API would need to be documented in the manpage.

@pfliu
Copy link
Author

pfliu commented Aug 26, 2019

Sorry for the delay.

I think we need a mechanism to free the old mask eventually. Otherwise if some application calls this API frequently there might be a unlimited long term memory leak. But I don't see a way to free it. The only reasonable way I can think of is to add a list and reuse masks with the same value. That would guarantee that any leak is bounded at least to all possible CPU masks. Drawback is that it will require locking, so will add more dependencies to libnuma.

I don't like the term "outdated". Can you just use "update" ?

The new API would need to be documented in the manpage.
Appreciate for your review.

Adopt your suggestion about naming and manpage. But I have a different point on memory leak issue.
I can not figure out it and my patch seems to release mask.
E.g.
if (node_cpu_mask_v2[node]) {
if (node_cpu_mask_v2_stale) {
copy_bitmask_to_bitmask(mask, node_cpu_mask_v2[node]);
node_cpu_mask_v2_stale = 0;
numa_bitmask_free(mask); <-- release
mask = NULL;
}

So I only see the risk of broken node_cpu_mask_v2[] between readers and updaters. It can be fixed with the price of locking, but considering cpu offline/online is not frequently, it is worth to do?

Thanks,
Pingfan

numa_node_to_cpus() caches the cpu to node mapping, and not updates it
during the cpu online/offline event.

Ideally, in order to update the mapping automatically, it requires
something like udev to spy on kernel event socket, and update cache if
event happen. This solution is a little complicated inside a libnuma.so. In
stead of doing so, exposing an API numa_node_to_cpu_outdated() for user,
and saddling the event-detecting task to the user.

So the user of libnuma can work using either of the following models:
 -1. blindless outdate cache if careless about performance
     numa_node_to_cpu_outdated();
     numa_node_to_cpu();
 -2. event driven spy on kernel event, if happened, call
     numa_node_to_cpu_outdated();

Signed-off-by: Pingfan Liu <piliu@redhat.com>
@pfliu
Copy link
Author

pfliu commented Sep 16, 2019

This is v2, which uses atomic on node_cpu_mask_v2_stale to protect the race issue, which causes the potential memory leak issue.

I mistook to make a new pull request from numactl/master<- pfliu/numactl/master by #77. And I closed it and switch back to numactl/master<- pfliu/numactl/nocache

Thanks,
Pingfan

@andikleen
Copy link
Contributor

Looks good now. Thanks.

@andikleen andikleen merged commit 3cc2e00 into numactl:master Sep 16, 2019
@pfliu pfliu deleted the nocache branch November 5, 2019 08:38
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.

2 participants