Re: [PATCH net-next 05/22] cxgb4: Add T5 write combining support
On 03/13/13 08:43, David Laight wrote: From my recollection of the x86 architecture, the memory barriers are hardly ever needed, certainly not in the places where, for example a ppc needs them. I'd actually suspect that the normal wmb() for x86 should be a nop. About the only place where any on the fence instructions are needed are in relation to write combining accesses. In particular I don't believe they are ever needed to synchronise uncached accesses with each other, or with cached accesses (which are snooped). David The question at hand is how should we indicate that we're finished with a Write Combined set of stores in an architecturally neutral manner? Is wmb() a good approach? Vipul has noted that the iWarp code uses a new "wc_wmb()" for this purpose which seems to be the same _currently_ as wmb() for all current platforms. I presume that the iWarp folks pick a new name to offer themselves some cover in the future. And yes, the code sequence that was accidentally included in Vipul's previous patches should never have been submitted for inclusion in kernel.org. It got missed in our internal reviews and I apologize for that. Casey -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH net-next 05/22] cxgb4: Add T5 write combining support
> >>> + writel(n, adap->bar2 + q->udb + 8);
> >>> +#if defined(CONFIG_X86_32) || defined(CONFIG_X86_64)
> >>> + asm volatile("sfence" : : : "memory");
> >>> +#endif
> >> There is absolutely no way I'm letting anyone put crap like this
> >> into a driver.
> >>
> >> Use a portable inteface, and if one does not exist create one.
> >
> > I guess you'll have to add a wc_wmb() function for all the hw platforms
> > supported by the kernel. I see libibverbs defines this for the user
> > side in include/infiniband/arch.h, and that could be used as the meat of
> > the hw platform-specific implementations.
> >
> I see that normal wmb() code for x86_64 architecture is same as what
> above #ifdef condition is doing. To be more clear I looked at the
> assembly code for wmb and find that it is converted into 'sfence'
> instruction. So, I think above code should be replaced with wmb call
> which will also take care of portability on different architecture. I
> will submit the series again soon.
>From my recollection of the x86 architecture, the memory barriers
are hardly ever needed, certainly not in the places where, for example
a ppc needs them. I'd actually suspect that the normal wmb() for
x86 should be a nop.
About the only place where any on the fence instructions are needed
are in relation to write combining accesses.
In particular I don't believe they are ever needed to synchronise
uncached accesses with each other, or with cached accesses (which
are snooped).
David
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 05/22] cxgb4: Add T5 write combining support
On 12-03-2013 20:12, Steve Wise wrote:
> On 3/12/2013 7:19 AM, David Miller wrote:
>> From: Vipul Pandya
>> Date: Tue, 12 Mar 2013 17:16:17 +0530
>>
>>> + writel(n, adap->bar2 + q->udb + 8);
>>> +#if defined(CONFIG_X86_32) || defined(CONFIG_X86_64)
>>> + asm volatile("sfence" : : : "memory");
>>> +#endif
>> There is absolutely no way I'm letting anyone put crap like this
>> into a driver.
>>
>> Use a portable inteface, and if one does not exist create one.
>
> I guess you'll have to add a wc_wmb() function for all the hw platforms
> supported by the kernel. I see libibverbs defines this for the user
> side in include/infiniband/arch.h, and that could be used as the meat of
> the hw platform-specific implementations.
>
I see that normal wmb() code for x86_64 architecture is same as what
above #ifdef condition is doing. To be more clear I looked at the
assembly code for wmb and find that it is converted into 'sfence'
instruction. So, I think above code should be replaced with wmb call
which will also take care of portability on different architecture. I
will submit the series again soon.
I would like to request RDMA/cxgb4 and csiostor driver maintainer to
review the series if it has not been done already. It would be great If
I can include review comments from them in my next version of series.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 05/22] cxgb4: Add T5 write combining support
On 3/12/2013 7:19 AM, David Miller wrote:
From: Vipul Pandya
Date: Tue, 12 Mar 2013 17:16:17 +0530
+ writel(n, adap->bar2 + q->udb + 8);
+#if defined(CONFIG_X86_32) || defined(CONFIG_X86_64)
+ asm volatile("sfence" : : : "memory");
+#endif
There is absolutely no way I'm letting anyone put crap like this
into a driver.
Use a portable inteface, and if one does not exist create one.
I guess you'll have to add a wc_wmb() function for all the hw platforms
supported by the kernel. I see libibverbs defines this for the user
side in include/infiniband/arch.h, and that could be used as the meat of
the hw platform-specific implementations.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 05/22] cxgb4: Add T5 write combining support
From: Vipul Pandya
Date: Tue, 12 Mar 2013 17:16:17 +0530
> + writel(n, adap->bar2 + q->udb + 8);
> +#if defined(CONFIG_X86_32) || defined(CONFIG_X86_64)
> + asm volatile("sfence" : : : "memory");
> +#endif
There is absolutely no way I'm letting anyone put crap like this
into a driver.
Use a portable inteface, and if one does not exist create one.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
