Date:	Thu, 12 Mar 2015 04:49:33 +0000
From:	KY Srinivasan <kys@...rosoft.com>
To:	KY Srinivasan <kys@...rosoft.com>, Jason Wang <jasowang@...hat.com>
CC:	"olaf@...fle.de" <olaf@...fle.de>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"apw@...onical.com" <apw@...onical.com>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"davem@...emloft.net" <davem@...emloft.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: devel [mailto:driverdev-devel-bounces@...uxdriverproject.org] On
> Behalf Of KY Srinivasan
> Sent: Wednesday, March 11, 2015 8:32 PM
> To: Jason Wang
> Cc: olaf@...fle.de; netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> gregkh@...uxfoundation.org; apw@...onical.com;
> devel@...uxdriverproject.org; davem@...emloft.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@...hat.com]
> > Sent: Wednesday, March 11, 2015 8:08 PM
> > To: KY Srinivasan
> > Cc: davem@...emloft.net; netdev@...r.kernel.org; linux-
> > kernel@...r.kernel.org; devel@...uxdriverproject.org; olaf@...fle.de;
> > apw@...onical.com; gregkh@...uxfoundation.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@...rosoft.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@...hat.com> for pointing
> > > out this issue.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@...rosoft.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@...uxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists