On Fri, Mar 24, 2023 at 06:03:37AM +0000, Song Liu wrote: > > > > On Mar 23, 2023, at 8:30 PM, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > [...] > > > > > The Uptodate flag check needs to be done by the caller; the > > find_get_page() family return !uptodate pages. > > > > But find_get_page() does not advertise itself as NMI-safe. And I > > think it's wrong to try to make it NMI-safe. Most of the kernel is > > not NMI-safe. I think it's incumbent on the BPF people to get the > > information they need ahead of taking the NMI. NMI handlers are not > > supposed to be doing a huge amount of work! I don't really understand > > why it needs to do work in NMI context; surely it can note the location of > > the fault and queue work to be done later (eg on irq-enable, task-switch > > or return-to-user) > > The use case here is a profiler (similar to perf-record). Parsing the > build id in side the NMI makes the profiler a lot simpler. Otherwise, > we will need some post processing for each sample. Simpler for you, maybe. But this is an NMI! It's not supposed to be doing printf-formatting or whatever, much less poking around in the file cache. Like perf, it should record a sample and then convert that later. Maybe it can defer to a tasklet, but i think scheduling work is a better option. > OTOH, it is totally fine if build_id_parse() fails some time, say < 5%. > The profiler output is still useful in such cases. > > I guess the next step is to replace find_get_page() with a NMI-safe > version? No, absolutely not. Stop doing so much work in an NMI.
