[Qemu-devel] [PATCH] xen: Drop net_rx_ok

[Qemu-devel] [PATCH] xen: Drop net_rx_ok

[Fam Zheng]

This is necessary because once we return false from .can_receive, we
need to flush the queue when the .can_receive conditions become true
again, (for example when more buffer is available).

We can rely on net_rx_packet (which checks the same conditions) to drop
the packet if the device is not ready, so drop net_xen_info.can_receive.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/xen_nic.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 19ecfc4..da9edc0 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -234,27 +234,6 @@ static void net_rx_response(struct XenNetDev *netdev,
 
 #define NET_IP_ALIGN 2
 
-static int net_rx_ok(NetClientState *nc)
-{
-    struct XenNetDev *netdev = qemu_get_nic_opaque(nc);
-    RING_IDX rc, rp;
-
-    if (netdev->xendev.be_state != XenbusStateConnected) {
-        return 0;
-    }
-
-    rc = netdev->rx_ring.req_cons;
-    rp = netdev->rx_ring.sring->req_prod;
-    xen_rmb();
-
-    if (rc == rp || RING_REQUEST_CONS_OVERFLOW(&netdev->rx_ring, rc)) {
-        xen_be_printf(&netdev->xendev, 2, "%s: no rx buffers (%d/%d)\n",
-                      __FUNCTION__, rc, rp);
-        return 0;
-    }
-    return 1;
-}
-
 static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size)
 {
     struct XenNetDev *netdev = qemu_get_nic_opaque(nc);
@@ -304,7 +283,6 @@ static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size
 static NetClientInfo net_xen_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = net_rx_ok,
     .receive = net_rx_packet,
 };
 
-- 
2.4.4

Re: [Qemu-devel] [PATCH] xen: Drop net_rx_ok

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

On Tue, Jun 30, 2015 at 10:42:37AM +0800, Fam Zheng wrote:
> This is necessary because once we return false from .can_receive, we
> need to flush the queue when the .can_receive conditions become true
> again, (for example when more buffer is available).
> 
> We can rely on net_rx_packet (which checks the same conditions) to drop
> the packet if the device is not ready, so drop net_xen_info.can_receive.

This patch changes behavior:

Previously can_receive() false meant packets are queued.

Now those same conditions result in net_rx_packet() returning -1, so
packets are discarded.

In order to keep the spirit of the queuing mechanism - where we tell a
sender to hold off until more rx buffers become available - I think the
following line in net_rx_packet() needs to be changed:

  if (rc == rp || RING_REQUEST_CONS_OVERFLOW(&netdev->rx_ring, rc)) {
      xen_be_printf(&netdev->xendev, 2, "no buffer, drop packet\n");
      return -1;  <-- this should be changed to return 0
  }

That change assumes that net_event()'s flush is always called when the
rx ring gets more free space.

Any thoughts from Xen folks?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] xen: Drop net_rx_ok

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

On Thu, Jul 02, 2015 at 01:39:16PM +0100, Stefan Hajnoczi wrote:
> On Tue, Jun 30, 2015 at 10:42:37AM +0800, Fam Zheng wrote:
> > This is necessary because once we return false from .can_receive, we
> > need to flush the queue when the .can_receive conditions become true
> > again, (for example when more buffer is available).
> > 
> > We can rely on net_rx_packet (which checks the same conditions) to drop
> > the packet if the device is not ready, so drop net_xen_info.can_receive.
> 
> This patch changes behavior:
> 
> Previously can_receive() false meant packets are queued.
> 
> Now those same conditions result in net_rx_packet() returning -1, so
> packets are discarded.
> 
> In order to keep the spirit of the queuing mechanism - where we tell a
> sender to hold off until more rx buffers become available - I think the
> following line in net_rx_packet() needs to be changed:
> 
>   if (rc == rp || RING_REQUEST_CONS_OVERFLOW(&netdev->rx_ring, rc)) {
>       xen_be_printf(&netdev->xendev, 2, "no buffer, drop packet\n");
>       return -1;  <-- this should be changed to return 0
>   }
> 
> That change assumes that net_event()'s flush is always called when the
> rx ring gets more free space.
> 
> Any thoughts from Xen folks?

Ping?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] xen: Drop net_rx_ok

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]

On Mon, Jul 20, 2015 at 06:12:09PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jul 02, 2015 at 01:39:16PM +0100, Stefan Hajnoczi wrote:
> > On Tue, Jun 30, 2015 at 10:42:37AM +0800, Fam Zheng wrote:
> > > This is necessary because once we return false from .can_receive, we
> > > need to flush the queue when the .can_receive conditions become true
> > > again, (for example when more buffer is available).
> > > 
> > > We can rely on net_rx_packet (which checks the same conditions) to drop
> > > the packet if the device is not ready, so drop net_xen_info.can_receive.
> > 
> > This patch changes behavior:
> > 
> > Previously can_receive() false meant packets are queued.
> > 
> > Now those same conditions result in net_rx_packet() returning -1, so
> > packets are discarded.
> > 
> > In order to keep the spirit of the queuing mechanism - where we tell a
> > sender to hold off until more rx buffers become available - I think the
> > following line in net_rx_packet() needs to be changed:
> > 
> >   if (rc == rp || RING_REQUEST_CONS_OVERFLOW(&netdev->rx_ring, rc)) {
> >       xen_be_printf(&netdev->xendev, 2, "no buffer, drop packet\n");
> >       return -1;  <-- this should be changed to return 0
> >   }
> > 
> > That change assumes that net_event()'s flush is always called when the
> > rx ring gets more free space.
> > 
> > Any thoughts from Xen folks?
> 
> Ping?

Need input from Xen developers.

Ping?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] xen: Drop net_rx_ok

[Chen Gang]

On 7/27/15 21:16, Stefan Hajnoczi wrote:
> On Mon, Jul 20, 2015 at 06:12:09PM +0100, Stefan Hajnoczi wrote:
>> On Thu, Jul 02, 2015 at 01:39:16PM +0100, Stefan Hajnoczi wrote:
>>> On Tue, Jun 30, 2015 at 10:42:37AM +0800, Fam Zheng wrote:
>>>> This is necessary because once we return false from .can_receive, we
>>>> need to flush the queue when the .can_receive conditions become true
>>>> again, (for example when more buffer is available).
>>>>
>>>> We can rely on net_rx_packet (which checks the same conditions) to drop
>>>> the packet if the device is not ready, so drop net_xen_info.can_receive.
>>>
>>> This patch changes behavior:
>>>
>>> Previously can_receive() false meant packets are queued.
>>>
>>> Now those same conditions result in net_rx_packet() returning -1, so
>>> packets are discarded.
>>>
>>> In order to keep the spirit of the queuing mechanism - where we tell a
>>> sender to hold off until more rx buffers become available - I think the
>>> following line in net_rx_packet() needs to be changed:
>>>
>>>   if (rc == rp || RING_REQUEST_CONS_OVERFLOW(&netdev->rx_ring, rc)) {
>>>       xen_be_printf(&netdev->xendev, 2, "no buffer, drop packet\n");
>>>       return -1;  <-- this should be changed to return 0
>>>   }
>>>
>>> That change assumes that net_event()'s flush is always called when the
>>> rx ring gets more free space.
>>>
>>> Any thoughts from Xen folks?
>>
>> Ping?
> 
> Need input from Xen developers.
> 
> Ping?
> 

In fact, I am not Xen folks. But I shall provide my idea for it, since
Cc to me.

For me, it is reasonable to use "return 0" instead of "return -1".

 - It completes the current API and keep compatible with the original
   usage. As return size, it needs to consider about all numbers (zero,
   negative, and positive numbers, like e.g. standard read/write API).

 - Comparing with all the other NetClientInfo.receive providers, some of
   them consider about it (e.g. virtio), others not (maybe each of them
   have their own reasons -- e.g. hardware will never generate 0 size).

Welcome any other members' ideas, suggestions and completions.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH] xen: Drop net_rx_ok

[Stefano Stabellini]

On Mon, 27 Jul 2015, Stefan Hajnoczi wrote:
> On Mon, Jul 20, 2015 at 06:12:09PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Jul 02, 2015 at 01:39:16PM +0100, Stefan Hajnoczi wrote:
> > > On Tue, Jun 30, 2015 at 10:42:37AM +0800, Fam Zheng wrote:
> > > > This is necessary because once we return false from .can_receive, we
> > > > need to flush the queue when the .can_receive conditions become true
> > > > again, (for example when more buffer is available).
> > > > 
> > > > We can rely on net_rx_packet (which checks the same conditions) to drop
> > > > the packet if the device is not ready, so drop net_xen_info.can_receive.
> > > 
> > > This patch changes behavior:
> > > 
> > > Previously can_receive() false meant packets are queued.
> > > 
> > > Now those same conditions result in net_rx_packet() returning -1, so
> > > packets are discarded.
> > > 
> > > In order to keep the spirit of the queuing mechanism - where we tell a
> > > sender to hold off until more rx buffers become available - I think the
> > > following line in net_rx_packet() needs to be changed:
> > > 
> > >   if (rc == rp || RING_REQUEST_CONS_OVERFLOW(&netdev->rx_ring, rc)) {
> > >       xen_be_printf(&netdev->xendev, 2, "no buffer, drop packet\n");
> > >       return -1;  <-- this should be changed to return 0
> > >   }
> > > 
> > > That change assumes that net_event()'s flush is always called when the
> > > rx ring gets more free space.
> > > 
> > > Any thoughts from Xen folks?
> > 
> > Ping?
> 
> Need input from Xen developers.

FYI I don't have an opinion on this: I am not familiar with this code as
xen_nic is the one Xen PV backend in QEMU that is not used with Xen.
Theoretically it could be used, but actually there is no way to select
it from the toolstack.