lkml.org 
[lkml]   [2015]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in the signalling logic with kick_q
Date


> -----Original Message-----
> From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> Behalf Of KY Srinivasan
> Sent: Wednesday, March 11, 2015 8:32 PM
> To: Jason Wang
> Cc: olaf@aepfle.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> gregkh@linuxfoundation.org; apw@canonical.com;
> devel@linuxdriverproject.org; davem@davemloft.net
> Subject: RE: [PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in the
> signalling logic with kick_q
>
>
>
> > -----Original Message-----
> > From: Jason Wang [mailto:jasowang@redhat.com]
> > Sent: Wednesday, March 11, 2015 8:08 PM
> > To: KY Srinivasan
> > Cc: davem@davemloft.net; netdev@vger.kernel.org; linux-
> > kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> > apw@canonical.com; gregkh@linuxfoundation.org; KY Srinivasan
> > Subject: Re: [PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in
> > the signalling logic with kick_q
> >
> >
> >
> > On Thu, Mar 12, 2015 at 3:04 AM, K. Y. Srinivasan <kys@microsoft.com>
> > wrote:
> > > When the caller specifies that signalling should be deferred, we
> > > need to address the case where we are not able to place the current
> > > packet because the buffer is full. In this case, we will signal the
> > > host as some packets may have been placed on the ring buffer.
> > > I would like to thank Jason Wang <jasowang@redhat.com> for pointing
> > > out this issue.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > ---
> > > drivers/hv/channel.c | 32 ++++++++++++++++++++++++++++++++
> > > 1 files changed, 32 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index
> > > e58cdb7..ae06ba9 100644
> > > --- a/drivers/hv/channel.c
> > > +++ b/drivers/hv/channel.c
> > > @@ -614,8 +614,24 @@ int vmbus_sendpacket_ctl(struct
> vmbus_channel
> > > *channel, void *buffer,
> > >
> > > ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3,
> > > &signal);
> > >
> > > + /*
> > > + * Here is the logic for signalling the host:
> > > + * 1. If the host is already draining the ringbuffer,
> > > + * don't signal. This is indicated by the parameter
> > > + * "signal".
> > > + *
> > > + * 2. If we are not able to write, signal if kick_q is false.
> > > + * kick_q being false indicates that we may have placed zero or
> > > + * more packets with more packets to come. We will signal in
> > > + * this case even if potentially we may have not placed any
> > > + * packet. This is a rare enough condition that it should not
> > > + * matter.
> > > + */
> > > +
> > > if ((ret == 0) && kick_q && signal)
> > > vmbus_setevent(channel);
> > > + else if ((ret != 0) && !kick_q)
> > > + vmbus_setevent(channel);
> > >
> > > return ret;
> > > }
> > > @@ -705,8 +721,24 @@ int vmbus_sendpacket_pagebuffer_ctl(struct
> > > vmbus_channel *channel,
> > >
> > > ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3,
> > > &signal);
> > >
> > > + /*
> > > + * Here is the logic for signalling the host:
> > > + * 1. If the host is already draining the ringbuffer,
> > > + * don't signal. This is indicated by the parameter
> > > + * "signal".
> > > + *
> > > + * 2. If we are not able to write, signal if kick_q is false.
> > > + * kick_q being false indicates that we may have placed zero or
> > > + * more packets with more packets to come. We will signal in
> > > + * this case even if potentially we may have not placed any
> > > + * packet. This is a rare enough condition that it should not
> > > + * matter.
> > > + */
> > > +
> > > if ((ret == 0) && kick_q && signal)
> > > vmbus_setevent(channel);
> > > + else if ((ret != 0) && !kick_q)
> > > + vmbus_setevent(channel);
> > >
> > > return ret;
> > > }
> > > --
> >
> > Looks like we need to kick unconditionally here. Consider we may get -
> > EAGAIN when we want to send the last skb (kick_q is true) from the
> > list. We need kick host in this case.
> >
> > Btw, another method is let the driver to decide e.g exporting the
> > vmbus_setevent() and call it in netvsc_start_xmit().
>
> There is other state that governs if the host needs to be signaled and I don't
> think we want to expose that to netvsc. In any case, netvsc will have to signal
> the host when EGAIN is received.
> We might as well do it in the vmbus driver.
>
> Thank you Jason, I will respin and resubmit the series.

Looks like there may be a way to make the signaling more precise even in this case.
I am going to experiment with it.

K. Y
>
> K. Y
> >
> >
> >
>
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


\
 
 \ /
  Last update: 2015-03-12 06:01    [W:2.649 / U:0.800 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site