On Thu, Nov 17, 2022 at 08:06:21AM -0500, Joel Fernandes wrote: > > > > On Nov 17, 2022, at 7:58 AM, Uladzislau Rezki <urezki@xxxxxxxxx> wrote: > > > > On Wed, Nov 16, 2022 at 10:05:46PM +0000, Joel Fernandes wrote: > >>> On Wed, Nov 16, 2022 at 7:19 PM Uladzislau Rezki <urezki@xxxxxxxxx> wrote: > >>> > >>> Hello, Paul, Joel. > >>> > >>>>> > >>>>> Yes sure, I am doing a run now with my patch. However, I have a > >>>>> question -- why do you feel blocking in the kworker is not an issue? > >>>>> You are taking a snapshot before queuing the normal kwork and then > >>>>> reading the snapshot when the normal kwork runs. Considering it is a > >>>>> high priority queue, the delay between when you are taking the > >>>>> snapshot, and reading it is likely small so there is a bigger chance > >>>>> of blocking in cond_synchronize_rcu(). Did I miss something? > >>>>> > >>>> We can wait indeed in the reclaim worker. But the worker does not do any > >>>> nasty or extra work here. If there is a need we block and wait. After a > >>>> grace period, we are awoken and proceed. > >>>> > >>>> Therefore i do not see the reason in handling two cases: > >>>> > >>>> if (gp_done) > >>>> queue_work(); > >>>> else > >>>> queue_rcu_work(); > >>>> > >>>> it is the same if we just queue the work and check on entry. The current > >>>> scenario is: queue the work after a grace period. This is the difference. > >>>> > >>>> Right if the reclaimer was a high prio kthread a time would be shorter. > >>>> > >>>> In your scenario the time seems even shorter(i have not checked) because > >>>> you update a snapshot of krcp each time a kvfree_rcu() is invoked. So > >>>> basically even though you have objects whose grace period is passed you > >>>> do not separate it anyhow. Because you update the: > >>>> > >>>> krcp->gp_snap = get_state_synchronize_rcu(); > >>>> > >>>> too often. > >>>> > >>> Once upon a time we discussed that it is worth to keep track of GP > >>> per-a-page in order to reduce a memory footprint. Below patch addresses > >>> it: > >> > >> In the patch below, it appears you are tracking the GP per krwp, and > >> not per page. But I could be missing something - could you split it > >> into separate patches for easier review? > >> > > I will split. I was thinking about it. The GP is tracked per-a-page. As for > > krwp it is only for channel_3. Everything goes there if no-page or no cache. > > > Ah, ok. > > >> > >> Also it still does cond_synchronize_rcu() :-( > >> > > Sometimes we need to wait for a GP we can not just release :) > > You know that is not what I meant ;) I was concerned about the blocking. > Let me split. After that we/you can test and check if there is any issue with sleeping on entry for waiting a GP if needed. -- Uladzislau Rezki