On Tue, May 26, 2020 at 01:11:57PM -0700, Jakub Kicinski wrote:
> On Tue, 26 May 2020 11:33:09 -0400 Johannes Weiner wrote:
> > On Wed, May 20, 2020 at 05:24:11PM -0700, Jakub Kicinski wrote:
> > > + penalty_jiffies += calculate_high_delay(memcg, nr_pages,
> > > + swap_find_max_overage(memcg));
> > > +
> > > /*
> > > * Clamp the max delay per usermode return so as to still keep the
> > > * application moving forwards and also permit diagnostics, albeit
> > > @@ -2585,12 +2608,25 @@ static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
> > > * reclaim, the cost of mismatch is negligible.
> > > */
> > > do {
> > > - if (page_counter_is_above_high(&memcg->memory)) {
> > > - /* Don't bother a random interrupted task */
> > > - if (in_interrupt()) {
> > > + bool mem_high, swap_high;
> > > +
> > > + mem_high = page_counter_is_above_high(&memcg->memory);
> > > + swap_high = page_counter_is_above_high(&memcg->swap);
> >
> > Please open-code these checks instead - we don't really do getters and
> > predicates for these, and only have the setters because they are more
> > complicated operations.
>
> I added this helper because the calculation doesn't fit into 80 chars.
>
> In particular reclaim_high will need a temporary variable or IMHO
> questionable line split.
>
> static void reclaim_high(struct mem_cgroup *memcg,
> unsigned int nr_pages,
> gfp_t gfp_mask)
> {
> do {
> if (!page_counter_is_above_high(&memcg->memory))
> continue;
> memcg_memory_event(memcg, MEMCG_HIGH);
> try_to_free_mem_cgroup_pages(memcg, nr_pages, gfp_mask, true);
> } while ((memcg = parent_mem_cgroup(memcg)) &&
> !mem_cgroup_is_root(memcg));
> }
>
> What's your preference? Mine is a helper, but I'm probably not
> sensitive enough to the ontology here :)
if (page_counter_read(&memcg->memory) <
READ_ONCE(memcg->memory.high))
continue;
should work fine. It's the same formatting in mem_cgroup_swap_full():
if (page_counter_read(&memcg->swap) * 2 >=
READ_ONCE(memcg->swap.max))
