Skip to content

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

Merged
andikleen merged 1 commit into
numactl:masterfrom
pfliu:nocache
Sep 16, 2019
Merged

libnuma: introduce an API to outdate cpu to node mapping#73
andikleen merged 1 commit into
numactl:masterfrom
pfliu:nocache

Conversation

@pfliu

@pfliu pfliu commented Jul 23, 2019

Copy link
Copy Markdown

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

pfliu commented Jul 23, 2019

Copy link
Copy Markdown
Author

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

pfliu commented Jul 29, 2019

Copy link
Copy Markdown
Author

Could anybody help to review it?
Thanks,
Pingfan

@andikleen

Copy link
Copy Markdown
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

pfliu commented Aug 26, 2019

Copy link
Copy Markdown
Author

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

pfliu commented Sep 16, 2019

Copy link
Copy Markdown
Author

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
Copy Markdown
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