* [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers
@ 2011-07-19 18:37 Shirley Ma
2011-07-19 19:09 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Shirley Ma @ 2011-07-19 18:37 UTC (permalink / raw)
To: David Miller, mst; +Cc: netdev, jasowang
Signed-off-by: Shirley Ma <xma@us.ibm.com>
---
drivers/vhost/net.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 70ac604..83cb738 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -189,8 +189,10 @@ static void handle_tx(struct vhost_net *net)
break;
}
/* If more outstanding DMAs, queue the work */
- if (unlikely(vq->upend_idx - vq->done_idx >
- VHOST_MAX_PEND)) {
+ if (unlikely((vq->upend_idx - vq->done_idx >
+ VHOST_MAX_PEND) ||
+ (vq->upend_idx - vq->done_idx >
+ VHOST_MAX_PEND - UIO_MAXIOV))) {
tx_poll_start(net, sock);
set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
break;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers
2011-07-19 18:37 [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers Shirley Ma
@ 2011-07-19 19:09 ` Michael S. Tsirkin
2011-07-19 20:56 ` Shirley Ma
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2011-07-19 19:09 UTC (permalink / raw)
To: Shirley Ma; +Cc: David Miller, netdev, jasowang
On Tue, Jul 19, 2011 at 11:37:58AM -0700, Shirley Ma wrote:
> Signed-off-by: Shirley Ma <xma@us.ibm.com>
> ---
>
> drivers/vhost/net.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 70ac604..83cb738 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -189,8 +189,10 @@ static void handle_tx(struct vhost_net *net)
> break;
> }
> /* If more outstanding DMAs, queue the work */
> - if (unlikely(vq->upend_idx - vq->done_idx >
> - VHOST_MAX_PEND)) {
> + if (unlikely((vq->upend_idx - vq->done_idx >
> + VHOST_MAX_PEND) ||
> + (vq->upend_idx - vq->done_idx >
> + VHOST_MAX_PEND - UIO_MAXIOV))) {
Could you please explain why this makes sense please?
VHOST_MAX_PEND is 128 UIO_MAXIOV is 1024 so
the result is negative?
I thought upend_idx - done_idx is exactly the number
of buffers, so once we get too many we stop until
one gets freed?
> tx_poll_start(net, sock);
> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> break;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers
2011-07-19 19:09 ` Michael S. Tsirkin
@ 2011-07-19 20:56 ` Shirley Ma
2011-07-20 10:28 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Shirley Ma @ 2011-07-19 20:56 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: David Miller, netdev, jasowang
On Tue, 2011-07-19 at 22:09 +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 19, 2011 at 11:37:58AM -0700, Shirley Ma wrote:
> > Signed-off-by: Shirley Ma <xma@us.ibm.com>
> > ---
> >
> > drivers/vhost/net.c | 6 ++++--
> > 1 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 70ac604..83cb738 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -189,8 +189,10 @@ static void handle_tx(struct vhost_net *net)
> > break;
> > }
> > /* If more outstanding DMAs, queue the work */
> > - if (unlikely(vq->upend_idx - vq->done_idx >
> > - VHOST_MAX_PEND)) {
> > + if (unlikely((vq->upend_idx - vq->done_idx >
> > + VHOST_MAX_PEND) ||
> > + (vq->upend_idx - vq->done_idx >
> > + VHOST_MAX_PEND -
> UIO_MAXIOV))) {
>
> Could you please explain why this makes sense please?
> VHOST_MAX_PEND is 128 UIO_MAXIOV is 1024 so
> the result is negative?
I thought it is equal to:
if (vq->upend_idx > vq->done_idx)
check vq->upend_idx - vq->done_idx > VHOST_MAX_PEND
if (vq->upend_idx < vq->done_idx)
check vq->upend_idx + UIO_MAXIOV - vq->done_idx > VHOST_MAX_PEND
> I thought upend_idx - done_idx is exactly the number
> of buffers, so once we get too many we stop until
> one gets freed?
They are index, so in vhost zerocopy callback, we can get the idx right
away.
>
> > tx_poll_start(net, sock);
> > set_bit(SOCK_ASYNC_NOSPACE,
> &sock->flags);
> > break;
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers
2011-07-19 20:56 ` Shirley Ma
@ 2011-07-20 10:28 ` Michael S. Tsirkin
2011-07-20 15:43 ` Shirley Ma
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2011-07-20 10:28 UTC (permalink / raw)
To: Shirley Ma; +Cc: David Miller, netdev, jasowang
On Tue, Jul 19, 2011 at 01:56:25PM -0700, Shirley Ma wrote:
> On Tue, 2011-07-19 at 22:09 +0300, Michael S. Tsirkin wrote:
> > On Tue, Jul 19, 2011 at 11:37:58AM -0700, Shirley Ma wrote:
> > > Signed-off-by: Shirley Ma <xma@us.ibm.com>
> > > ---
> > >
> > > drivers/vhost/net.c | 6 ++++--
> > > 1 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 70ac604..83cb738 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -189,8 +189,10 @@ static void handle_tx(struct vhost_net *net)
> > > break;
> > > }
> > > /* If more outstanding DMAs, queue the work */
> > > - if (unlikely(vq->upend_idx - vq->done_idx >
> > > - VHOST_MAX_PEND)) {
> > > + if (unlikely((vq->upend_idx - vq->done_idx >
> > > + VHOST_MAX_PEND) ||
> > > + (vq->upend_idx - vq->done_idx >
> > > + VHOST_MAX_PEND -
> > UIO_MAXIOV))) {
> >
> > Could you please explain why this makes sense please?
> > VHOST_MAX_PEND is 128 UIO_MAXIOV is 1024 so
> > the result is negative?
>
> I thought it is equal to:
>
> if (vq->upend_idx > vq->done_idx)
> check vq->upend_idx - vq->done_idx > VHOST_MAX_PEND
> if (vq->upend_idx < vq->done_idx)
> check vq->upend_idx + UIO_MAXIOV - vq->done_idx > VHOST_MAX_PEND
>
Check it out: upend_idx == done_idx == 0 does not satisfy the
above conditions but does trigger in your code, right?
Better keep it simple. Maybe:
if (unlikely(vq->upend_idx - vq->done_idx > VHOST_MAX_PEND) ||
(unlikely(vq->upend_idx < vq->done_idx) &&
unlikely(vq->upend_idx + UIO_MAXIOV - vq->done_idx >
VHOST_MAX_PEND)))
?
Also, please add commit log documenting what does the patch
fix: something like:
'the test for # of outstanding buffers returned
incorrect results when due to wrap around,
upend_idx < done_idx'?
> > I thought upend_idx - done_idx is exactly the number
> > of buffers, so once we get too many we stop until
> > one gets freed?
>
> They are index, so in vhost zerocopy callback, we can get the idx right
> away.
>
> >
> > > tx_poll_start(net, sock);
> > > set_bit(SOCK_ASYNC_NOSPACE,
> > &sock->flags);
> > > break;
> > >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers
2011-07-20 10:28 ` Michael S. Tsirkin
@ 2011-07-20 15:43 ` Shirley Ma
2011-07-20 16:17 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Shirley Ma @ 2011-07-20 15:43 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: David Miller, netdev, jasowang
On Wed, 2011-07-20 at 13:28 +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 19, 2011 at 01:56:25PM -0700, Shirley Ma wrote:
> > On Tue, 2011-07-19 at 22:09 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jul 19, 2011 at 11:37:58AM -0700, Shirley Ma wrote:
> > > > Signed-off-by: Shirley Ma <xma@us.ibm.com>
> > > > ---
> > > >
> > > > drivers/vhost/net.c | 6 ++++--
> > > > 1 files changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > > index 70ac604..83cb738 100644
> > > > --- a/drivers/vhost/net.c
> > > > +++ b/drivers/vhost/net.c
> > > > @@ -189,8 +189,10 @@ static void handle_tx(struct vhost_net
> *net)
> > > > break;
> > > > }
> > > > /* If more outstanding DMAs, queue the
> work */
> > > > - if (unlikely(vq->upend_idx - vq->done_idx
> >
> > > > - VHOST_MAX_PEND)) {
> > > > + if (unlikely((vq->upend_idx - vq->done_idx
> >
> > > > + VHOST_MAX_PEND) ||
> > > > + (vq->upend_idx - vq->done_idx
> >
> > > > + VHOST_MAX_PEND -
> > > UIO_MAXIOV))) {
> > >
> > > Could you please explain why this makes sense please?
> > > VHOST_MAX_PEND is 128 UIO_MAXIOV is 1024 so
> > > the result is negative?
> >
> > I thought it is equal to:
> >
> > if (vq->upend_idx > vq->done_idx)
> > check vq->upend_idx - vq->done_idx > VHOST_MAX_PEND
> > if (vq->upend_idx < vq->done_idx)
> > check vq->upend_idx + UIO_MAXIOV - vq->done_idx >
> VHOST_MAX_PEND
> >
>
> Check it out: upend_idx == done_idx == 0 does not satisfy the
> above conditions but does trigger in your code, right?
We don't hit upend_idx == done_idx == 0. Only upend_idx == done_idx ==
UIO_MAXIOV could happen if the lower device has issue and never DMA any
packets out.
> Better keep it simple. Maybe:
>
> if (unlikely(vq->upend_idx - vq->done_idx > VHOST_MAX_PEND) ||
> (unlikely(vq->upend_idx < vq->done_idx) &&
> unlikely(vq->upend_idx + UIO_MAXIOV - vq->done_idx >
> VHOST_MAX_PEND)))
>
> ?
>
> Also, please add commit log documenting what does the patch
> fix: something like:
> 'the test for # of outstanding buffers returned
> incorrect results when due to wrap around,
> upend_idx < done_idx'?
Sure, will modify it and resubmit.
> > > I thought upend_idx - done_idx is exactly the number
> > > of buffers, so once we get too many we stop until
> > > one gets freed?
> >
> > They are index, so in vhost zerocopy callback, we can get the idx
> right
> > away.
> >
> > >
> > > > tx_poll_start(net, sock);
> > > > set_bit(SOCK_ASYNC_NOSPACE,
> > > &sock->flags);
> > > > break;
> > > >
Thanks
Shirley
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers
2011-07-20 15:43 ` Shirley Ma
@ 2011-07-20 16:17 ` Michael S. Tsirkin
2011-07-20 16:49 ` Shirley Ma
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2011-07-20 16:17 UTC (permalink / raw)
To: Shirley Ma; +Cc: David Miller, netdev, jasowang
On Wed, Jul 20, 2011 at 08:43:09AM -0700, Shirley Ma wrote:
> On Wed, 2011-07-20 at 13:28 +0300, Michael S. Tsirkin wrote:
> > On Tue, Jul 19, 2011 at 01:56:25PM -0700, Shirley Ma wrote:
> > > On Tue, 2011-07-19 at 22:09 +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Jul 19, 2011 at 11:37:58AM -0700, Shirley Ma wrote:
> > > > > Signed-off-by: Shirley Ma <xma@us.ibm.com>
> > > > > ---
> > > > >
> > > > > drivers/vhost/net.c | 6 ++++--
> > > > > 1 files changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > > > index 70ac604..83cb738 100644
> > > > > --- a/drivers/vhost/net.c
> > > > > +++ b/drivers/vhost/net.c
> > > > > @@ -189,8 +189,10 @@ static void handle_tx(struct vhost_net
> > *net)
> > > > > break;
> > > > > }
> > > > > /* If more outstanding DMAs, queue the
> > work */
> > > > > - if (unlikely(vq->upend_idx - vq->done_idx
> > >
> > > > > - VHOST_MAX_PEND)) {
> > > > > + if (unlikely((vq->upend_idx - vq->done_idx
> > >
> > > > > + VHOST_MAX_PEND) ||
> > > > > + (vq->upend_idx - vq->done_idx
> > >
> > > > > + VHOST_MAX_PEND -
> > > > UIO_MAXIOV))) {
> > > >
> > > > Could you please explain why this makes sense please?
> > > > VHOST_MAX_PEND is 128 UIO_MAXIOV is 1024 so
> > > > the result is negative?
> > >
> > > I thought it is equal to:
> > >
> > > if (vq->upend_idx > vq->done_idx)
> > > check vq->upend_idx - vq->done_idx > VHOST_MAX_PEND
> > > if (vq->upend_idx < vq->done_idx)
> > > check vq->upend_idx + UIO_MAXIOV - vq->done_idx >
> > VHOST_MAX_PEND
> > >
> >
> > Check it out: upend_idx == done_idx == 0 does not satisfy the
> > above conditions but does trigger in your code, right?
>
> We don't hit upend_idx == done_idx == 0. Only upend_idx == done_idx ==
> UIO_MAXIOV could happen if the lower device has issue and never DMA any
> packets out.
My point was that the logic isn't the same, even though
you said 'it is equal to'.
Same applies to upend_idx == 1, done_idx == 0.
> > Better keep it simple. Maybe:
> >
> > if (unlikely(vq->upend_idx - vq->done_idx > VHOST_MAX_PEND) ||
> > (unlikely(vq->upend_idx < vq->done_idx) &&
> > unlikely(vq->upend_idx + UIO_MAXIOV - vq->done_idx >
> > VHOST_MAX_PEND)))
> >
> > ?
> >
> > Also, please add commit log documenting what does the patch
> > fix: something like:
> > 'the test for # of outstanding buffers returned
> > incorrect results when due to wrap around,
> > upend_idx < done_idx'?
>
> Sure, will modify it and resubmit.
>
> > > > I thought upend_idx - done_idx is exactly the number
> > > > of buffers, so once we get too many we stop until
> > > > one gets freed?
> > >
> > > They are index, so in vhost zerocopy callback, we can get the idx
> > right
> > > away.
> > >
> > > >
> > > > > tx_poll_start(net, sock);
> > > > > set_bit(SOCK_ASYNC_NOSPACE,
> > > > &sock->flags);
> > > > > break;
> > > > >
>
> Thanks
> Shirley
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers
2011-07-20 16:17 ` Michael S. Tsirkin
@ 2011-07-20 16:49 ` Shirley Ma
0 siblings, 0 replies; 7+ messages in thread
From: Shirley Ma @ 2011-07-20 16:49 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: David Miller, netdev, jasowang
On Wed, 2011-07-20 at 19:17 +0300, Michael S. Tsirkin wrote:
> My point was that the logic isn't the same, even though
> you said 'it is equal to'.
> Same applies to upend_idx == 1, done_idx == 0.
:) not sure what I was thinking.
I will resubmit the patch.
Thanks
Shirley
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-07-20 16:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-19 18:37 [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers Shirley Ma
2011-07-19 19:09 ` Michael S. Tsirkin
2011-07-19 20:56 ` Shirley Ma
2011-07-20 10:28 ` Michael S. Tsirkin
2011-07-20 15:43 ` Shirley Ma
2011-07-20 16:17 ` Michael S. Tsirkin
2011-07-20 16:49 ` Shirley Ma
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).