Thanks Vlastimil !! On 6/17/2021 8:07 PM, Vlastimil Babka wrote: > On 6/17/21 9:30 AM, Charan Teja Kalla wrote: >> Thanks Vlastimil for your inputs!! >> >> On 6/16/2021 5:29 PM, Vlastimil Babka wrote: >>>> This triggering of proactive compaction is done on a write to >>>> sysctl.compaction_proactiveness by user. >>>> >>>> [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=facdaa917c4d5a376d09d25865f5a863f906234a >>>> >>>> Signed-off-by: Charan Teja Reddy <charante@xxxxxxxxxxxxxx> >>>> --- >>>> changes in V2: >>> You forgot to also summarize the changes. Please do in next version. >> >> I think we can get rid off 'proactive_defer' thread variable with the >> timeout approach you suggested. But it is still requires to have one >> additional variable 'proactive_compact_trigger', which main purpose is >> to decide if the kcompactd wakeup is for proactive compaction or not. >> Please see below code: >> if (wait_event_freezable_timeout() && !proactive_compact_trigger) { >> // do the non-proactive work >> continue >> } >> // do the proactive work >> ................. >> >> Thus I feel that on writing new proactiveness, it is required to do >> wakeup_kcomppactd() + set a flag that this wakeup is for proactive work. >> >> Am I failed to get your point here? > > The check whether to do non-proactive work is already guarded by > kcompactd_work_requested(), which looks at pgdat->kcompactd_max_order and this > is set by wakeup_kcompactd(). > > So with a plain wakeup where we don't set pgdat->kcompactd_max_order will make > it consider proactive work instead and we don't need another trigger variable > AFAICS. The wait_event/freezable_timeout() documentation says that: * Returns: * 0 if the @condition evaluated to %false after the @timeout elapsed, or * 1 if the @condition evaluated to %true after the @timeout elapsed, * or the remaining jiffies (at least 1) if the @condition evaluated * to %true before the @timeout elapsed. which means the condition must be evaluated to true or timeout should be elapsed for the function wait_event_freezable_timeout() to return. Please check the macro implementation of __wait_event, where it will be in for(;;) till the condition is evaluated to true or timeout happens. #define __wait_event_freezable_timeout(wq_head, condition, timeout) ___wait_event(wq_head, ___wait_cond_timeout(condition), TASK_INTERRUPTIBLE, 0, timeout, __ret = freezable_schedule_timeout(__ret)) Thus the plain wakeup of kcompactd don't do the proactive compact work. And so we should identify its wakeup for proactive work with a separate flag. > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
