linux-kernel - Re: [ltt-dev] [PATCH] Fix dirty page accounting in redirty_page_for

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 1 May 2009 17:19:11 -0400
From:	Mathieu Desnoyers <compudj@...stal.dyndns.org>
To:	Christoph Lameter <cl@...ux.com>
Cc:	Nick Piggin <nickpiggin@...oo.com.au>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Yuriy Lalym <ylalym@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	ltt-dev@...ts.casi.polymtl.ca, Tejun Heo <tj@...nel.org>,
	Ingo Molnar <mingo@...e.hu>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [ltt-dev] [PATCH] Fix dirty page accounting in
	redirty_page_for_writepage()

* Christoph Lameter (cl@...ux.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/