Re: [ltt-dev] [PATCH] Fix dirty page accounting in redirty_page_for_writepage()

[Mathieu Desnoyers <compudj@krystal.dyndns.org>]

* Christoph Lameter (cl@linux.com) wrote:
> On Fri, 1 May 2009, Mathieu Desnoyers wrote:
> 
> > Then do you have a better idea on how to deal with
> > __inc_zone_state/inc_zone_state without duplicating the code and without
> > adding useless local_irq_save/restore on x86 ?
> 
> We are already using __inc_zone_state to avoid local_irq_save/restore.
> 

Then, if I understand you correctly, you propose :

void __inc_zone_state(struct zone *zone, enum zone_stat_item item)
{
        ... assuming p references percpu "u8" counters ...
        u8 p_new;

        p_new = percpu_add_return(p, 1);

        if (unlikely(!(p_new & pcp->stat_threshold_mask)))
                zone_page_state_add(pcp->stat_threshold, zone, item);
}

void inc_zone_state(struct zone *zone, enum zone_stat_item item)
{
        ... assuming p references percpu "u8" counters ...
        u8 p_new;

        p_new = percpu_add_return_irqsafe(p, 1);

        if (unlikely(!(p_new & pcp->stat_threshold_mask)))
                zone_page_state_add(pcp->stat_threshold, zone, item);
}

(therefore opting for code duplication)

Am I correct ?

It can indeed by argued to be more straightforward than adding irq
disabling primitives which behave differently depending on the
architecture. And it certainly makes sense as long as duplicated
functions are small enough and as long as we never touch more than one
counter in the same function.

A compromise would be to include the appropriate irq disabling or
preempt disabling in the "standard version" of these functions, but
create underscore prefixed versions which would need
percpu_local_irqsave and friends to be done separately. Therefore, the
standard usage would be easy to deal with and review, and it would still
allow using the underscore-prefixed version when code would otherwise
have to be duplicated or when multiple counters must be updated at once.

And the rule would be simple enough :

- percpu_add_return_irqsafe is safe wrt interrupts.

- _always_ disable interrupts or use percpu_local_irqsave/restore
  around __percpu_add_return_irqsafe().

For the example above, this would let us write :

void __inc_zone_state(struct zone *zone, enum zone_stat_item item)
{
        ... assuming p references percpu "u8" counters ...
        u8 p_new;

        p_new = __percpu_add_return_irqsafe(p, 1);

        if (unlikely(!(p_new & pcp->stat_threshold_mask)))
                zone_page_state_add(pcp->stat_threshold, zone, item);
}

void inc_zone_state(struct zone *zone, enum zone_stat_item item)
{
	unsigned long flags;

	percpu_local_irqsave(flags);
	__inc_zone_state(zone, item);
	percpu_local_irqrestore(flags);
}

Which is more compact and does not duplicate code.

Mathieu

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68