All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: Ming Lei <ming.lei@canonical.com>
Cc: Will Deacon <will.deacon@arm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Mark Salter <msalter@redhat.com>
Subject: Re: [PATCH 0/3] RFC: addition to DMA API
Date: Thu, 1 Sep 2011 11:42:23 -0400 (EDT)	[thread overview]
Message-ID: <Pine.LNX.4.44L0.1109011127080.1896-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <CACVXFVOT9FZexiZhuJtvyFP8qOPYUKg0JqGarBJgT4R8SN7p5Q@mail.gmail.com>

On Thu, 1 Sep 2011, Ming Lei wrote:

> First, most of barriers have made this kind of flush not necessary, as
> explained int the example above.

You don't seem to understand the difference between memory barriers and 
write flushes.  Neither makes the other unnecessary -- they do 
different things.

Now, there is good reason to question why write flushes should be 
needed at all.  According to the definition in 
Documentation/DMA-API.txt (the document doesn't distinguish between 
coherent memory and consistent memory):

  Consistent memory is memory for which a write by either the device or
  the processor can immediately be read by the processor or device
  without having to worry about caching effects.  (You may however need
  to make sure to flush the processor's write buffers before telling
  devices to read that memory.)

As far as I can tell, we are talking about a cache flush rather than a 
processor write buffer flush.  If that's so, it would appear that the 
memory in question is not truly coherent.

>  Even for ehci driver, the flush to be
> added is only __one__ special case, so could you list other cases in
> which the flush operation is a must and memory barrier can't do it?

There are other places in ehci-hcd where write flushes would help
improve performance or correctness, although the one in qh_append_tds()  
is probably the most critical.  One such place is in qh_link_async();
others are in qh_link_periodic(), qh_unlink_periodic(), itd_link_urb(),
and sitd_link_urb().

And there are similar places in the other USB host controller drivers.

> If one can understand dma master access order on shared memory
> in the context, it is not difficult to use memory barrier to avoid the
> flush operation.

That simply is not true.  If it were, you would not have needed to 
submit your original patch.

> Second, as I said before, the flush operation is very easy to cause dma
> descriptor updated in partial, as qtd descriptor updated in the ehci case.

No, not if memory barriers are used correctly.

> It is one of the side effects of the flush to be introduced.  Fortunately, EHCI
> can handle this correctly, but can other hardwares always handle this correctly?

If they can't, they are already broken.  Adding write flushes won't 
make them any worse.

Alan Stern


WARNING: multiple messages have this Message-ID (diff)
From: stern@rowland.harvard.edu (Alan Stern)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 0/3] RFC: addition to DMA API
Date: Thu, 1 Sep 2011 11:42:23 -0400 (EDT)	[thread overview]
Message-ID: <Pine.LNX.4.44L0.1109011127080.1896-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <CACVXFVOT9FZexiZhuJtvyFP8qOPYUKg0JqGarBJgT4R8SN7p5Q@mail.gmail.com>

On Thu, 1 Sep 2011, Ming Lei wrote:

> First, most of barriers have made this kind of flush not necessary, as
> explained int the example above.

You don't seem to understand the difference between memory barriers and 
write flushes.  Neither makes the other unnecessary -- they do 
different things.

Now, there is good reason to question why write flushes should be 
needed at all.  According to the definition in 
Documentation/DMA-API.txt (the document doesn't distinguish between 
coherent memory and consistent memory):

  Consistent memory is memory for which a write by either the device or
  the processor can immediately be read by the processor or device
  without having to worry about caching effects.  (You may however need
  to make sure to flush the processor's write buffers before telling
  devices to read that memory.)

As far as I can tell, we are talking about a cache flush rather than a 
processor write buffer flush.  If that's so, it would appear that the 
memory in question is not truly coherent.

>  Even for ehci driver, the flush to be
> added is only __one__ special case, so could you list other cases in
> which the flush operation is a must and memory barrier can't do it?

There are other places in ehci-hcd where write flushes would help
improve performance or correctness, although the one in qh_append_tds()  
is probably the most critical.  One such place is in qh_link_async();
others are in qh_link_periodic(), qh_unlink_periodic(), itd_link_urb(),
and sitd_link_urb().

And there are similar places in the other USB host controller drivers.

> If one can understand dma master access order on shared memory
> in the context, it is not difficult to use memory barrier to avoid the
> flush operation.

That simply is not true.  If it were, you would not have needed to 
submit your original patch.

> Second, as I said before, the flush operation is very easy to cause dma
> descriptor updated in partial, as qtd descriptor updated in the ehci case.

No, not if memory barriers are used correctly.

> It is one of the side effects of the flush to be introduced.  Fortunately, EHCI
> can handle this correctly, but can other hardwares always handle this correctly?

If they can't, they are already broken.  Adding write flushes won't 
make them any worse.

Alan Stern

  reply	other threads:[~2011-09-01 15:42 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-08-31 21:30 [PATCH 0/3] RFC: addition to DMA API Mark Salter
2011-08-31 21:30 ` Mark Salter
2011-08-31 21:30 ` [PATCH 1/3] add dma_coherent_write_sync " Mark Salter
2011-08-31 21:30   ` Mark Salter
2011-09-01  2:59   ` Josh Cartwright
2011-09-01  2:59     ` Josh Cartwright
2011-09-01  9:57   ` Michał Mirosław
2011-09-01  9:57     ` Michał Mirosław
2011-09-01 12:36     ` Mark Salter
2011-09-01 12:36       ` Mark Salter
2011-09-06 14:30       ` Catalin Marinas
2011-09-06 14:30         ` Catalin Marinas
2011-08-31 21:30 ` [PATCH 2/3] define ARM-specific dma_coherent_write_sync Mark Salter
2011-08-31 21:30   ` Mark Salter
2011-09-06 14:32   ` Catalin Marinas
2011-09-06 14:32     ` Catalin Marinas
2011-09-06 14:37     ` Mark Salter
2011-09-06 14:37       ` Mark Salter
2011-09-06 14:48       ` Catalin Marinas
2011-09-06 14:48         ` Catalin Marinas
2011-09-06 15:02         ` Mark Salter
2011-09-06 15:02           ` Mark Salter
2011-10-03  1:40           ` Jon Masters
2011-10-03  1:40             ` Jon Masters
2011-10-03  8:44             ` Catalin Marinas
2011-10-03  8:44               ` Catalin Marinas
2011-10-03  9:24               ` Jon Masters
2011-10-03  9:24                 ` Jon Masters
2011-08-31 21:30 ` [PATCH 3/3] add dma_coherent_write_sync calls to USB EHCI driver Mark Salter
2011-08-31 21:30   ` Mark Salter
2011-09-01  2:33   ` Ming Lei
2011-09-01  2:33     ` Ming Lei
2011-09-01  2:09 ` [PATCH 0/3] RFC: addition to DMA API Ming Lei
2011-09-01  2:09   ` Ming Lei
2011-09-01  3:09   ` Alan Stern
2011-09-01  3:09     ` Alan Stern
2011-09-01  3:41     ` Ming Lei
2011-09-01  3:41       ` Ming Lei
2011-09-01  8:45       ` Will Deacon
2011-09-01  8:45         ` Will Deacon
2011-09-01  9:14         ` Ming Lei
2011-09-01  9:14           ` Ming Lei
2011-09-01 15:42           ` Alan Stern [this message]
2011-09-01 15:42             ` Alan Stern
2011-09-01 16:04             ` Russell King - ARM Linux
2011-09-01 16:04               ` Russell King - ARM Linux
2011-09-01 17:31               ` Will Deacon
2011-09-01 17:31                 ` Will Deacon
2011-09-01 18:07                 ` Russell King - ARM Linux
2011-09-01 18:07                   ` Russell King - ARM Linux
2011-09-01 19:14                 ` Mark Salter
2011-09-01 19:14                   ` Mark Salter
2011-09-01 15:22       ` Alan Stern
2011-09-01 15:22         ` Alan Stern
2011-09-01 15:56         ` Ming Lei
2011-09-01 15:56           ` Ming Lei
2011-09-01 16:48           ` Alan Stern
2011-09-01 16:48             ` Alan Stern
2011-09-02  0:59             ` Ming Lei
2011-09-02  0:59               ` Ming Lei
2011-09-02 13:53               ` Alan Stern
2011-09-02 13:53                 ` Alan Stern
2011-09-01  9:11 ` Will Deacon
2011-09-01  9:11   ` Will Deacon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Pine.LNX.4.44L0.1109011127080.1896-100000@iolanthe.rowland.org \
    --to=stern@rowland.harvard.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ming.lei@canonical.com \
    --cc=msalter@redhat.com \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.