lkml.org 
[lkml]   [2019]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC v3] writeback: add elastic bdi in cgwb bdp
From
Date
On 14.11.19 10:38, Hillf Danton wrote:
>
> On Tue, 12 Nov 2019 17:02:36 -0800 Andrew Morton wrote:
>>
>> On Tue, 12 Nov 2019 11:42:27 +0800 Hillf Danton <hdanton@sina.com> wrote:
>>>
>>> The elastic bdi (ebdi) which is the mirror bdi of spinning disk,
>>> SSD and USB key on market is introduced to balancing dirty pages
>>> (bdp).
>>>
>>> The risk arises that system runs out of free memory, when dirty
>>> pages are produced too many too soon, so bdp is needed in field.
>>>
>>> Ebdi facilitates bdp in elastic time intervals e.g. from a jiffy
>>> to one HZ, depending on the time it would take to increase dirty
>>> pages by the amount which is defined by the variable
>>> ratelimit_pages.
>>>
>>> During cgroup writeback (cgwb) bdp, ebdi helps observe the
>>> changes both in cgwb's dirty pages (dirty speed) and in
>>> written-out pages (laundry speed) in elastic time intervals,
>>> until a balance is established between the two parties i.e.
>>> the two speeds statistically equal.
>>>
>>> The above mechanism of elastic equilibrium effectively prevents
>>> dirty page hogs, as no chance is left for dirty pages to pile up,
>>> thus cuts the risk that system free memory falls to unsafe level.
>>>
>>> Thanks to Rong Chen for testing.
>>
>> That sounds like a Tested-by:
>>
> Yes, Sir, will add Tested-by: Rong Chen <rong.a.chen@intel.com>
>
>> The changelog has no testing results. Please prepare results which
>> show, amongst other things, the change in performance when the kernel
>> isn't tight on memory. As well as the alteration in behaviour when
>> memory is short.
>>
> Will do.
>
>> Generally, please work on making this code much more understandable?
>>
> Will do.
>
>>>
>>> ...
>>>
>>> --- a/fs/fs-writeback.c
>>> +++ b/fs/fs-writeback.c
>>> @@ -811,6 +811,8 @@ static long wb_split_bdi_pages(struct bd
>>> if (nr_pages == LONG_MAX)
>>> return LONG_MAX;
>>>
>>> + return nr_pages;
>>> +
>>> /*
>>> * This may be called on clean wb's and proportional distribution
>>> * may not make sense, just use the original @nr_pages in those
>>> @@ -1604,6 +1606,7 @@ static long writeback_chunk_size(struct
>>> pages = min(pages, work->nr_pages);
>>> pages = round_down(pages + MIN_WRITEBACK_PAGES,
>>> MIN_WRITEBACK_PAGES);
>>> + pages = work->nr_pages;
>>
>> It's unclear what this is doing, but it makes the three preceding
>> statements non-operative.
>>
> This change, and the above one as well, is trying to bypass the
> current bandwidth, and a couple of rounds of cleanup are needed
> after it survives the LTP.
>
>>> }
>>>
>>> return pages;
>>> @@ -2092,6 +2095,9 @@ void wb_workfn(struct work_struct *work)
>>> wb_wakeup_delayed(wb);
>>>
>>> current->flags &= ~PF_SWAPWRITE;
>>> +
>>> + if (waitqueue_active(&wb->bdp_waitq))
>>> + wake_up_all(&wb->bdp_waitq);
>>
>> Please add a comment explaining why this is being done here.
>>
> After writing out some dirty pages, it it a check point to see if
> a balance is already set up between the dirty speed and laundry
> speed. Those under throttling will be unthrottled after seeing
> a balance in place.
>
> A comment will be added.
>
>>> }
>>>
>>> /*
>>> --- a/mm/page-writeback.c
>>> +++ b/mm/page-writeback.c
>>> @@ -1830,6 +1830,67 @@ pause:
>>> wb_start_background_writeback(wb);
>>> }
>>>
>>> +/**
>>> + * cgwb_bdp_should_throttle() tell if a wb should be throttled
>>> + * @wb bdi_writeback to throttle
>>> + *
>>> + * To avoid the risk of exhausting the system free memory, we check
>>> + * and try much to prevent too many dirty pages from being produced
>>> + * too soon.
>>> + *
>>> + * For cgroup writeback, it is essencially to keep an equilibrium
>>
>> "it is essential"?
>>
> Yes Sir.
>
>>> + * between its dirty speed and laundry speed i.e. dirty pages are
>>> + * written out as fast as they are produced in an ideal state.
>>> + */
>>> +static bool cgwb_bdp_should_throttle(struct bdi_writeback *wb)
>>> +{
>>> + struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
>>> +
>>> + if (fatal_signal_pending(current))
>>> + return false;
>>> +
>>> + gdtc.avail = global_dirtyable_memory();
>>> +
>>> + domain_dirty_limits(&gdtc);
>>> +
>>> + gdtc.dirty = global_node_page_state(NR_FILE_DIRTY) +
>>> + global_node_page_state(NR_UNSTABLE_NFS) +
>>> + global_node_page_state(NR_WRITEBACK);
>>> +
>>> + if (gdtc.dirty < gdtc.bg_thresh)
>>> + return false;
>>> +
>>> + if (!writeback_in_progress(wb))
>>> + wb_start_background_writeback(wb);
>>
>> This is a bit ugly. Something called "bool cgwb_bdp_should_throttle()"
>> shoiuld just check whether we should throttle. But here it is, also
>> initiating writeback. That's an inappropriate thing for this function
>> to do?
>>
> It is the current bdp behavior trying to keep dirty pages below the
> user-configurable background threshold by waking up flushers, because
> no dirty page will be sent to disk without flusher's efforts, please
> see 143dfe8611a6 ("writeback: IO-less balance_dirty_pages()").
>
> Will try to find some chance to pinch it out.
>
>> Also, we don't know *why* this is being done here, because there's no
>> code comment explaining the reasoning to us.
>>
> Will add a comment.
>
>>
>>> + if (gdtc.dirty < gdtc.thresh)
>>> + return false;
>>> +
>>> + /*
>>> + * throttle wb if there is the risk that wb's dirty speed is
>>> + * running away from its laundry speed, better with statistic
>>> + * error taken into account.
>>> + */
>>> + return wb_stat(wb, WB_DIRTIED) >
>>> + wb_stat(wb, WB_WRITTEN) + wb_stat_error();
>>> +}
>>> +
>>>
>>> ...
>>>
>>> @@ -1888,29 +1945,38 @@ void balance_dirty_pages_ratelimited(str
>>> * 1000+ tasks, all of them start dirtying pages at exactly the same
>>> * time, hence all honoured too large initial task->nr_dirtied_pause.
>>> */
>>> - p = this_cpu_ptr(&bdp_ratelimits);
>>> - if (unlikely(current->nr_dirtied >= ratelimit))
>>> - *p = 0;
>>> - else if (unlikely(*p >= ratelimit_pages)) {
>>> - *p = 0;
>>> - ratelimit = 0;
>>> - }
>>> + dirty = this_cpu_ptr(&bdp_ratelimits);
>>> +
>>> /*
>>> * Pick up the dirtied pages by the exited tasks. This avoids lots of
>>> * short-lived tasks (eg. gcc invocations in a kernel build) escaping
>>> * the dirty throttling and livelock other long-run dirtiers.
>>> */
>>> - p = this_cpu_ptr(&dirty_throttle_leaks);
>>> - if (*p > 0 && current->nr_dirtied < ratelimit) {
>>> - unsigned long nr_pages_dirtied;
>>> - nr_pages_dirtied = min(*p, ratelimit - current->nr_dirtied);
>>> - *p -= nr_pages_dirtied;
>>> - current->nr_dirtied += nr_pages_dirtied;
>>> + leak = this_cpu_ptr(&dirty_throttle_leaks);
>>> +
>>> + if (*dirty + *leak < ratelimit_pages) {
>>> + /*
>>> + * nothing to do as it would take some more time to
>>> + * eat out ratelimit_pages
>>> + */
>>> + try_bdp = false;
>>> + } else {
>>> + try_bdp = true;
>>> +
>>> + /*
>>> + * bdp in flight helps detect dirty page hogs soon
>>> + */
>>
>> How? Please expand on this comment a lot.
>>
> We should be cautious here in red zone after paying the ratelimit_pages
> price; we might soon have to tackle a deluge of dirty page hogs.
>
> Will cut it.
>
>>> + flights = this_cpu_ptr(&bdp_in_flight);
>>> +
>>> + if ((*flights)++ & 1) {
>>
>> What is that "& 1" doing?
>>
> It helps to tell if a bdp is alredy in flight.
>
> It would have been something like
>
> if (*flights == 0) {
> (*flights)++;
> } else {
> *flights = 0;
>>> + *dirty = *dirty + *leak - ratelimit_pages;
>>> + *leak = 0;
>>> + }
>
> but I was curious to see the flights in long run.
>
> Thanks
> Hillf
>
>>> }
>>> preempt_enable();
>>>
>>> - if (unlikely(current->nr_dirtied >= ratelimit))
>>> - balance_dirty_pages(wb, current->nr_dirtied);
>>> + if (try_bdp)
>>> + cgwb_bdp(wb);
>>>
>>> wb_put(wb);
>
>

Friendly note that your mail client is messing up the thread hierarchy
again (I think it was correct for a while):

Message-Id: <20191114093832.8504-1-hdanton@sina.com>
In-Reply-To: <20191112034227.3112-1-hdanton@sina.com>
References: <20191112034227.3112-1-hdanton@sina.com>

I assume a kernel developer can setup a mail client or switch to a sane
one. Please don't prove me wrong. ;)

--

Thanks,

David / dhildenb

\
 
 \ /
  Last update: 2019-11-14 14:24    [W:0.827 / U:0.716 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site