On Mon, Mar 14, 2022 at 5:57 AM Michal Koutný mkoutny@suse.com wrote:
Hi 2.
On Sat, Mar 12, 2022 at 07:07:15PM +0000, Shakeel Butt shakeelb@google.com wrote:
It is (b) that I am aiming for in this patch. At least (a) was not happening in the cloudflare experiments. Are you suggesting having a dedicated high priority wq would solve both (a) and (b)? [...]
We can't argue what's the effect of periodic only flushing so this newly introduced factor would inherit that too. I find it superfluous.
Sorry I didn't get your point. What is superfluous?
Let me retell my understanding. The current implementation flushes based on cumulated error and time. Your patch proposes conditioning the former with another time-based flushing, whose duration can be up to 2 times longer than the existing periodic flush.
Assuming the periodic flush is working, the reader won't see data older than 2 seconds, so the additional sync-flush after (possible) 4 seconds seems superfluous.
(In the case of periodic flush being stuck, I thought the factor 2=4s/2s was superfluous, another magic parameter.)
I'm comparing here your proposal vs no synchronous flushing in workingset_refault().
Do you have any strong concerns with the currect patch?
Does that clarify?
(I agree with your initial thesis this can be iterated before it evolves to everyone's satisfaction.)
Thanks Michal for the explanation. For the long term, I think all these batching can be made part of core rstat infrastructure and as generic as you have described. Also there is interest in using rstat from BPF, so generic batching would be needed there as well.