Re: [PATCH 02/12] DRBD: activity_log

[Philipp Reisner <philipp.reisner@linbit.com>]

On Wednesday 25 March 2009 11:46:41 Andi Kleen wrote:
> > > > +	mutex_lock(&mdev->md_io_mutex); /* protects md_io_buffer,
> > > > al_tr_cycle, ... */
> > >
> > > Doing checksumming inside a lock looks nasty.
> >
> > Well, that is a mutex, not a spinlock. We need to hold that lock here,
>
> Yes it's independent. If it takes a lot of CPU time you'll likely have
> a bottle neck. It's normally a bad idea to do anything CPU intensive
> under a lock covering more than your current limited object.
>

We are talking about this checksum and copy to IO page loop:

	for (i = 0; i < mx; i++) {
		extent_nr = lc_entry(mdev->act_log,
				     mdev->al_tr_cycle+i)->lc_number;
		buffer->updates[i+1].pos = cpu_to_be32(mdev->al_tr_cycle+i);
		buffer->updates[i+1].extent = cpu_to_be32(extent_nr);
		xor_sum ^= extent_nr;
	}
	for (; i < AL_EXTENTS_PT; i++) {
		buffer->updates[i+1].pos = __constant_cpu_to_be32(-1);
		buffer->updates[i+1].extent = __constant_cpu_to_be32(LC_FREE);
		xor_sum ^= LC_FREE;
	}

Without being a CPU expert, my guess is that the single XOR instruction
is carried out while the CPU stalls waiting for the next load instruction
to complete.

Sorry, I still do not take the argument that this is "CPU intensive".

-Phil
-- 
: Dipl-Ing Philipp Reisner
: LINBIT | Your Way to High Availability
: Tel: +43-1-8178292-50, Fax: +43-1-8178292-82
: http://www.linbit.com

DRBD(R) and LINBIT(R) are registered trademarks of LINBIT, Austria.