Message-ID: <20130912134816.GA26417@Krystal>
Date:	Thu, 12 Sep 2013 09:48:16 -0400
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	John Stultz <john.stultz@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Richard Cochran <richardcochran@...il.com>,
	Prarit Bhargava <prarit@...hat.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org,
	lttng-dev@...ts.lttng.org
Subject: Re: [RFC PATCH] timekeeping: introduce timekeeping_is_busy()

* Peter Zijlstra (peterz@...radead.org) wrote:
> On Wed, Sep 11, 2013 at 11:22:52PM -0400, Mathieu Desnoyers wrote:
> > Cool!
> > 
> > Your design looks good to me. It reminds me of a latch. My only fear is
> > that struct timekeeper is probably too large to be copied every time on
> > the read path. Here is a slightly reworked version that would allow
> > in-place read of "foo" without copy.
> > 
> > struct foo {
> > 	...
> > };
> > 
> > struct latchfoo {
> > 	unsigned int head, tail;
> > 	spinlock_t write_lock;
> > 	struct foo data[2];
> > };
> > 
> > static
> > void foo_update(struct latchfoo *lf, void cb(struct foo *foo), void *ctx)
> > {
> > 	spin_lock(&lf->write_lock);
> > 	lf->head++;
> > 	smp_wmb();
> > 	lf->data[lf->head & 1] = lf->data[lf->tail & 1];
> > 	cb(&lf->data[lf->head & 1], ctx);
> 
> You do that initial copy such that the cb gets the previous state to
> work from and doesn't have to do a fetch/complete rewrite?

Yep, my original intent was to simplify life for callers.

> 
> The alternative is to give the cb function both pointers, old and new
> and have it do its thing.

Good point. The caller don't necessarily need to copy the old entry into
the new one: it may very well want to overwrite all the fields.

> 
> Yet another option is to split the update side into helper functions
> just like you did below for the read side.

OK. Updated code below.

> 
> > 	smp_wmb();
> > 	lf->tail++;
> > 	spin_unlock(&lock->write_lock);
> > }
> > 
> > static
> > unsigned int foo_read_begin(struct latchfoo *lf)
> > {
> > 	unsigned int ret;
> > 
> > 	ret = ACCESS_ONCE(lf->tail);
> > 	smp_rmb();
> > 	return ret;
> > }
> > 
> > static
> > struct foo *foo_read_get(struct latchfoo *lf, unsigned int tail)
> > {
> > 	return &lf->data[tail & 1];
> > }
> > 
> > static
> > int foo_read_retry(struct latchfoo *lf, unsigned int tail)
> > {
> > 	smp_rmb();
> > 	return (ACCESS_ONCE(lf->head) - tail >= 2);
> > }
> > 
> > Comments are welcome,
> 
> Yeah this would work. The foo_read_begin() and foo_read_get() split is a
> bit awkward but C doesn't really encourage us to do any better.

We might be able to do better:


struct foo {
	...
};

spinlock_t foo_lock;

struct latchfoo {
	unsigned int head, tail;
	struct foo data[2];
};

/**
 * foo_write_begin - begin foo update.
 *
 " @lf: struct latchfoo to update.
 * @prev: pointer to previous element (output parameter).
 * @next: pointer to next element (output parameter).
 *
 * The area pointed to by "next" should be considered uninitialized.
 * The caller needs to have exclusive update access to struct latchfoo.
 */
static
void foo_write_begin(struct latchfoo *lf, const struct foo **prev,
		struct foo **next)
{
	lf->head++;
	smp_wmb();
	*prev = &lf->data[lf->tail & 1];
	*next = &lf->data[lf->head & 1];
}

/**
 * foo_write_end - end foo update.
 *
 " @lf: struct latchfoo.
 *
 * The caller needs to have exclusive update access to struct latchfoo.
 */
static void
void foo_write_end(struct latchfoo *lf)
{
	smp_wmb();
	lf->tail++;
}

/**
 * foo_read_begin - begin foo read.
 *
 " @lf: struct latchfoo to read.
 * @tail: pointer to unsigned int containing tail position (output).
 */
static
struct foo *foo_read_begin(struct latchfoo *lf, unsigned int *tail)
{
	unsigned int ret;

	ret = ACCESS_ONCE(lf->tail);
	smp_rmb();
	*tail = ret;
	return &lf->data[ret & 1];
}

/**
 * foo_read_retry - end foo read, trigger retry if needed.
 *
 " @lf: struct latchfoo read.
 * @tail: tail position returned as output by foo_read_begin().
 *
 * If foo_read_retry() returns nonzero, the content of the read should
 * be considered invalid, and the read should be performed again to
 * reattempt reading coherent data, starting with foo_read_begin().
 */
static
int foo_read_retry(struct latchfoo *lf, unsigned int tail)
{
	smp_rmb();
	return (ACCESS_ONCE(lf->head) - tail >= 2);
}


Thoughts ?

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
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/

Powered by blists - more mailing lists