Re: [PATCH 15/15] Add DEFINE_SPUFS_ATTRIBUTE()

On Wed, 2007-09-12 at 10:47 +0200, Arnd Bergmann wrote:

On Wednesday 12 September 2007, Michael Ellerman wrote:

quoted

On Wed, 2007-09-12 at 17:43 +1000, Michael Ellerman wrote:

quoted

This patch adds DEFINE_SPUFS_ATTRIBUTE(), a wraper around
DEFINE_SIMPLE_ATTRIBUTE which does the specified locking for the get
routine for us.

Unfortunately we need two get routines (a locked and unlocked version) to
support the coredump code. This patch hides one of those (the locked version)
inside the macro foo.

quoted

jk said:

quoted

"Good god man!"

Yeah, I'm a bit lukewarm on this one. But the diffstat is nice, 50% code
reduction ain't bad :)

Have you looked at the change in object code size? I would expect the
object code to actually become bigger. I also think that it hurts
readability rather than help it.

Yeah I did, it's smaller actually:

   text    data     bss     dec     hex filename
  44898   17804     120   62822    f566 spufs-before.o
  44886   17804     120   62810    f55a spufs-after.o

Maybe a better solution is to change the core dump code to not
require the mutex to be held in the first place. By the time
we get to call the get functions, it should already be in
saved state and no longer be able to get scheduled, so we might
not actually need all the extra tricks with avoiding the
mutex to be taken again.

Well that'd be nice, but I don't see anywhere that that happens. AFAICT
the acquire we do in the first coredump callback is the first the SPU
contexts know about their PPE process dying. And spufs is still live, so
I think we definitely need to grab the mutex, or we might race with
userspace accessing spufs files.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person