On 05.08.2019 15:14, Andrew Cooper wrote: > On 05/08/2019 13:52, Jan Beulich wrote: >> On 30.07.2019 16:42, Andrew Cooper wrote: >>> c/s e9986b0dd "x86/vvmx: Simplify per-CPU memory allocations" had the wrong >>> indirection on its pointer check in nvmx_cpu_up_prepare(), causing the >>> VMCS-shadowing buffer never be allocated. Fix it. >>> >>> This in turn results in a massive quantity of logspam, as every virtual >>> vmentry/exit hits both gdprintk()s in the *_bulk() functions. >> The "in turn" here applies to the original bug (which gets fixed here) >> aiui, > > Correct. > >> i.e. there isn't any log spam with the fix in place anymore, is >> there? > > Incorrect, because... > >> If so, ... >> >>> Switch these to using printk_once(). The size of the buffer is chosen at >>> compile time, so complaining about it repeatedly is of no benefit. >> ... I'm not sure I'd agree with this move: Why would it be of interest >> only the first time that we (would have) overrun the buffer? > > ... we will either never overrun it, or overrun it on every virtual > vmentry/exit. > >> After all >> it's not only the compile time choice of buffer size that matters here, >> but also the runtime aspect of what value "n" has got passed into the >> functions. > > The few choices of "n" are fixed at compile time as well, which is why...
Oh - I should have looked at the callers. It's all ARRAY_SIZE(), indeed. >> If this is on the assumption that we'd want to know merely >> of the fact, not how often it occurs, then I'd think this ought to >> remain a debugging printk(). > > ... this still ends up as a completely unusable system, when the problem > occurs. > >> >>> Finally, drop the runtime NULL pointer checks. It is not terribly >>> appropriate >>> to be repeatedly checking infrastructure which is set up from start-of-day, >>> and in this case, actually hid the above bug. >> I don't see how the repeated checking would have hidden any bug: > > Without this check, Xen would have crashed with a NULL deference on the > original change, and highlighted the fact that the change was totally > broken. > > I didn't spot the issue because it was tested with a release build, > which is another reason why the replacement printk() is deliberately not > a debug-time-only. Taking this as a basis, there shouldn't be any debug-only printk()s. Anyway, with your explanations Reviewed-by: Jan Beulich <jbeul...@suse.com> Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
