Re: [PATCH 15/15] Add DEFINE_SPUFS_ATTRIBUTE()
On Thursday 13 September 2007, Michael Ellerman wrote: > 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. Right, I was only thinking about the dumping process itself, but there may be other processes that still have files open for that context. Arnd <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 15/15] Add DEFINE_SPUFS_ATTRIBUTE()
On Thursday 13 September 2007, Michael Ellerman wrote: 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. Right, I was only thinking about the dumping process itself, but there may be other processes that still have files open for that context. Arnd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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: > > On Wed, 2007-09-12 at 17:43 +1000, Michael Ellerman wrote: > > > 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. > > > > > jk said: > > > "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: textdata bss dec hex filename 44898 17804 120 62822f566 spufs-before.o 44886 17804 120 62810f55a 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 signature.asc Description: This is a digitally signed message part
Re: [PATCH 15/15] Add DEFINE_SPUFS_ATTRIBUTE()
On Wednesday 12 September 2007, Michael Ellerman wrote: > On Wed, 2007-09-12 at 17:43 +1000, Michael Ellerman wrote: > > 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. > > jk said: > > "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. 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. Arnd <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 15/15] Add DEFINE_SPUFS_ATTRIBUTE()
On Wed, 2007-09-12 at 17:43 +1000, Michael Ellerman wrote: > 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. > > Signed-off-by: Michael Ellerman <[EMAIL PROTECTED]> > --- > arch/powerpc/platforms/cell/spufs/file.c | 216 > +++--- > 1 files changed, 76 insertions(+), 140 deletions(-) jk said: > "Good god man!" Yeah, I'm a bit lukewarm on this one. But the diffstat is nice, 50% code reduction ain't bad :) 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 signature.asc Description: This is a digitally signed message part
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: On Wed, 2007-09-12 at 17:43 +1000, Michael Ellerman wrote: 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. jk said: 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: textdata bss dec hex filename 44898 17804 120 62822f566 spufs-before.o 44886 17804 120 62810f55a 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 signature.asc Description: This is a digitally signed message part
Re: [PATCH 15/15] Add DEFINE_SPUFS_ATTRIBUTE()
On Wed, 2007-09-12 at 17:43 +1000, Michael Ellerman wrote: 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. Signed-off-by: Michael Ellerman [EMAIL PROTECTED] --- arch/powerpc/platforms/cell/spufs/file.c | 216 +++--- 1 files changed, 76 insertions(+), 140 deletions(-) jk said: Good god man! Yeah, I'm a bit lukewarm on this one. But the diffstat is nice, 50% code reduction ain't bad :) 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 signature.asc Description: This is a digitally signed message part
Re: [PATCH 15/15] Add DEFINE_SPUFS_ATTRIBUTE()
On Wednesday 12 September 2007, Michael Ellerman wrote: On Wed, 2007-09-12 at 17:43 +1000, Michael Ellerman wrote: 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. jk said: 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. 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. Arnd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
