Hi Andrea,

it seems like a quite big problem since numactl wasn't prepared for cpu/node runtime changes between on/offline.

Do you need to disable all caching for libvirtd? Or just particular calls?

Hi Petr,

right now libvirt is just calling into libnuma, so we're not reacting in any
way to host topology changes, which can lead to weird output as evidenced in
the bug report.

The current implementation is quite puzzling: a short-lived utility like
numactl is probably going to query the data just once or twice, which makes a
cache not very useful; on the other hand, a long-lived process like libvirtd
is going to access the data several times and having a cache could really
speed things up, but the fact that the data in the cache might be outdated
kinda defeats the purpose.

Ideally libnuma should always report the current state of the host. I guess
there are two ways to ensure this is the case: get rid of the cache completely,
or rewrite it so that the data cointained in it is kept up-to-date. The latter
would be more difficult to get right, and I'm not sure the benefit would be
noticeable since we're talking about a really small amount of data here.

Please note that there might be other cached data in the library, and some of
it might be accessed directly insead of through a function call, which would
make both approaches impossible without an API change. I haven't investigated
this in depth.

Cheers.

Hi Andrea,

I've sent two patches upstream , but no acceptance so far (http://comments.gmane.org/gmane.linux.kernel.numa/934). Based on that, deferring the BZ to 7.3.

Hi Andrea,

I just takes the bug, and wonder how to fix it. Is it possible to add a udev rule by libvirt, so each time cpu online/offline, it will be notified by daemon. Then it re-call numactl to get the info. Or any other suggestion?

Thanks,
Pingfan

(In reply to Pingfan Liu from comment #11)
> Hi Andrea,
> 
> I just takes the bug, and wonder how to fix it. Is it possible to add a udev
> rule by libvirt, so each time cpu online/offline, it will be notified by
> daemon. Then it re-call numactl to get the info. Or any other suggestion?

libvirt doesn't spawn numactl, but rather it links against libnuma
directly, which means your suggestion wouldn't really work...

Personally I would just get rid of the cache, but as I understand it
that approach was rejected upstream the last time it was proposed.

Hi Andrea,

For the suggested method in comment#5, the maintainer cared about the performance issue, so we can not drop the cache. And I am thinking about other method like by which udev works. But before that, do you try the method in comment#5? And does it work?

I just want to assure "/sys/devices/system/node/node%d/cpumap" can reflect the online/offline cpu, not the present cpu.

Thanks,
  Pingfan

(In reply to Pingfan Liu from comment #13)
> Hi Andrea,
> 
> For the suggested method in comment#5, the maintainer cared about the
> performance issue, so we can not drop the cache. And I am thinking about
> other method like by which udev works. But before that, do you try the
> method in comment#5? And does it work?
> 
> I just want to assure "/sys/devices/system/node/node%d/cpumap" can reflect
> the online/offline cpu, not the present cpu.
> 
Check the kernel code, it reflect the mapping of possible cpu.

Hi Andrea,

What about exporting a API numa_node_to_cpu_outdate() to update the cache date after cpu online/offline?

You can get the suggested patch in attachment.

Thanks,
Pingfan

Created attachment 1590658 [details]
suggested patch to update the cpu to node mapping

Hi, sorry for taking a while to get back to you.

The patch looks okay (though I'd prefer if the function was called
numa_node_to_cpu_mark_outdated() or something like that), and
reacting to CPU topology changes in user code using udev rules
doesn't too terrible either.

That said, I still stand by what I said four years (!) ago in
Comment 4: the basic idea of caching this data in libnuma seems
misguided, considering that short-lived processes will not stick
around long enough for it to matter, and long-lived processes on the
other hand will be hindered by the fact that they're served stale
data.

I tracked down an alternative URL for the patch posting mentioned in
Comment 5[1] and it doesn't look to me like upstream has really
NACKed the suggested approach of ditching the cache but merely asked
for some mitigation, which Petr promptly implemented; the discussion
simply stopped after that.

How about re-submitting Petr's patches and see whether that results
in a better outcome this time around?


[1] https://www.spinics.net/lists/linux-numa/msg01134.html

(In reply to Andrea Bolognani from comment #17)
> Hi, sorry for taking a while to get back to you.
> 
> The patch looks okay (though I'd prefer if the function was called
> numa_node_to_cpu_mark_outdated() or something like that), and
> reacting to CPU topology changes in user code using udev rules
> doesn't too terrible either.
> 
> That said, I still stand by what I said four years (!) ago in
> Comment 4: the basic idea of caching this data in libnuma seems
> misguided, considering that short-lived processes will not stick
> around long enough for it to matter, and long-lived processes on the
> other hand will be hindered by the fact that they're served stale
> data.
Yes, that should be the best design. But it is unavoidable to introduce something like udev in libnuma, or to caller.
So I think a slight change is more possible to be accepted by maintainer
> 
> I tracked down an alternative URL for the patch posting mentioned in
> Comment 5[1] and it doesn't look to me like upstream has really
> NACKed the suggested approach of ditching the cache but merely asked
> for some mitigation, which Petr promptly implemented; the discussion
> simply stopped after that.
But I can not understand how a timeout can solve performance issue. (https://www.spinics.net/lists/linux-numa/msg01137.html)
On the other side, exporting a API numa_node_to_cpu_mark_outdated() can avoid the performance drop totally, considering online/offline cpu is not very frequent event on product environment.

> 
> How about re-submitting Petr's patches and see whether that results
> in a better outcome this time around?
> 
> 
> [1] https://www.spinics.net/lists/linux-numa/msg01134.html

I have created a pull request: https://github.com/numactl/numactl/pull/73
And add some comment about https://www.spinics.net/lists/linux-numa/msg01134.html

Thanks,
Pingfan

(In reply to Pingfan Liu from comment #19)
> I have created a pull request: https://github.com/numactl/numactl/pull/73

Alright, let's see how this goes :)

Good luck, and thanks for pushing this forward!

Hi Andrea,

The suggested patch has been merged and backport.
Could you have a test on https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=23531514 to see whether it works for you?

Thanks,
Pingfan

(In reply to Pingfan Liu from comment #21)
> Hi Andrea,
> 
> The suggested patch has been merged and backport.
> Could you have a test on
> https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=23531514 to see
> whether it works for you?

Hi,

unfortunately I don't have time to try this right now, but it's good
to know the API has made it upstream (and downstream)!

I'll try to find some time to add support for it in libvirt, but it
might be a while before I actually get around to it...

Hi,

Due to the schedule of 7.8, I think this fix should be tested as soon as possible.

Thanks,
Pingfan

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2020:1163