Messages in this thread | | | Date | Thu, 20 May 2010 14:37:11 -0700 | Subject | Re: [PATCH 01/10] x86 rwsem: minor cleanups | From | Michel Lespinasse <> |
| |
On Wed, May 19, 2010 at 4:47 AM, David Howells <dhowells@redhat.com> wrote: > Given that you describe this first, this would suggest that the subject of the > patch should be this. I'm not sure I'd count this as a minor cleanup. I think > I'd split it into its own patch.
OK, split it in two.
> Mostly okay, except where you said "expects old value in %edx" - that's only > true on i386, not x86_64. On the latter it would be %rdi. However, I can live > with that: it's true enough.
It's actually still %edx in x86_64 - we're calling into arch/x86/lib/rwsem_64.S which has its own unusual conventions.
>> - rwsem_count_t tmp; >> + rwsem_count_t tmp = -RWSEM_ACTIVE_WRITE_BIAS; >> ... >> : "+m" (sem->count), "=d" (tmp) >> - : "a" (sem), "1" (-RWSEM_ACTIVE_WRITE_BIAS) >> + : "a" (sem), "1" (tmp) > > If you're going to put the initialisation of EDX/RDI on tmp (which isn't really > necessary), rather than directly on the asm statement, you could change the > '"=d" (tmp)' output constraint to be '"+d" (tmp)' and drop the '"1" (tmp)' > constraint entirely.
I agree it'd be nicer, but I wondered if all gcc versions would handle the constraints change fine and then I chickened out. Instead I moved the initialization on the constraints list as was already done in __up_write(). All I was really shooting for here is consistency accross __down_write_nested, __up_read and __up_write functions.
> However, apart from that, feel free to add my Acked-by to this patch or its > split resultant patches.
Thanks. I'll send a V4 series soon integrating your feedback & mark the two splitted patches resulting from this one as Acked-by.
-- Michel "Walken" Lespinasse A program is never fully debugged until the last user dies. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |