[Qemu-devel] [PATCH] rocker: don't queue receive pkts when port is disabled

[Qemu-devel] [PATCH] rocker: don't queue receive pkts when port is disabled

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Commit 6e99c63 ("net/socket: Drop net_socket_can_send") changed the
semantics around .can_receive for sockets to now require the device to
flush queued pkts when transitioning to a .can_receive=true state.  Rocker
device was not flushing the queue on .can_receive=true transition, so the
receiver was stuck.

But, turns out we really don't want any queuing at all on the port when the
port is disabled, otherwise when the port transitions to enabled, we'd
receive and forward stale pkts that really should have been dropped.  So,
let's remove .can_receive so avoid queuing and drop the pkt in .receive if
the port is disabled.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 hw/net/rocker/rocker_fp.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
index d8d934c..ac53c13 100644
--- a/hw/net/rocker/rocker_fp.c
+++ b/hw/net/rocker/rocker_fp.c
@@ -125,18 +125,21 @@ int fp_port_eg(FpPort *port, const struct iovec *iov, int iovcnt)
     return ROCKER_OK;
 }
 
-static int fp_port_can_receive(NetClientState *nc)
-{
-    FpPort *port = qemu_get_nic_opaque(nc);
-
-    return port->enabled;
-}
-
 static ssize_t fp_port_receive_iov(NetClientState *nc, const struct iovec *iov,
                                    int iovcnt)
 {
     FpPort *port = qemu_get_nic_opaque(nc);
 
+    /* If the port is disabled, we want to drop this pkt
+     * now rather than queing it for later.  We don't want
+     * any stale pkts getting into the device when the port
+     * transitions to enabled.
+     */
+
+    if (!port->enabled) {
+        return iov_size(iov, iovcnt);
+    }
+
     return world_ingress(port->world, port->pport, iov, iovcnt);
 }
 
@@ -165,7 +168,6 @@ static void fp_port_set_link_status(NetClientState *nc)
 static NetClientInfo fp_port_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = fp_port_can_receive,
     .receive = fp_port_receive,
     .receive_iov = fp_port_receive_iov,
     .cleanup = fp_port_cleanup,
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] rocker: don't queue receive pkts when port is disabled

[Fam Zheng]

On Tue, 06/30 07:41, sfeldma@gmail.com wrote:
> From: Scott Feldman <sfeldma@gmail.com>
> 
> Commit 6e99c63 ("net/socket: Drop net_socket_can_send") changed the
> semantics around .can_receive for sockets to now require the device to
> flush queued pkts when transitioning to a .can_receive=true state.  Rocker
> device was not flushing the queue on .can_receive=true transition, so the
> receiver was stuck.
> 
> But, turns out we really don't want any queuing at all on the port when the
> port is disabled, otherwise when the port transitions to enabled, we'd
> receive and forward stale pkts that really should have been dropped.  So,
> let's remove .can_receive so avoid queuing and drop the pkt in .receive if
> the port is disabled.
> 
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> ---
>  hw/net/rocker/rocker_fp.c |   18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
> index d8d934c..ac53c13 100644
> --- a/hw/net/rocker/rocker_fp.c
> +++ b/hw/net/rocker/rocker_fp.c
> @@ -125,18 +125,21 @@ int fp_port_eg(FpPort *port, const struct iovec *iov, int iovcnt)
>      return ROCKER_OK;
>  }
>  
> -static int fp_port_can_receive(NetClientState *nc)
> -{
> -    FpPort *port = qemu_get_nic_opaque(nc);
> -
> -    return port->enabled;
> -}
> -
>  static ssize_t fp_port_receive_iov(NetClientState *nc, const struct iovec *iov,
>                                     int iovcnt)
>  {
>      FpPort *port = qemu_get_nic_opaque(nc);
>  
> +    /* If the port is disabled, we want to drop this pkt
> +     * now rather than queing it for later.  We don't want
> +     * any stale pkts getting into the device when the port
> +     * transitions to enabled.
> +     */
> +
> +    if (!port->enabled) {
> +        return iov_size(iov, iovcnt);

Other devices return -1 when dropping packets, for example see
e1000_receive_iov or virtio_net_receive. Maybe return -1 here too?

Fam


> +    }
> +
>      return world_ingress(port->world, port->pport, iov, iovcnt);
>  }
>  
> @@ -165,7 +168,6 @@ static void fp_port_set_link_status(NetClientState *nc)
>  static NetClientInfo fp_port_info = {
>      .type = NET_CLIENT_OPTIONS_KIND_NIC,
>      .size = sizeof(NICState),
> -    .can_receive = fp_port_can_receive,
>      .receive = fp_port_receive,
>      .receive_iov = fp_port_receive_iov,
>      .cleanup = fp_port_cleanup,
> -- 
> 1.7.10.4
> 

Re: [Qemu-devel] [PATCH] rocker: don't queue receive pkts when port is disabled

[Scott Feldman]

On Tue, Jun 30, 2015 at 6:47 PM, Fam Zheng <famz@redhat.com> wrote:
> On Tue, 06/30 07:41, sfeldma@gmail.com wrote:
>> From: Scott Feldman <sfeldma@gmail.com>
>>
>> Commit 6e99c63 ("net/socket: Drop net_socket_can_send") changed the
>> semantics around .can_receive for sockets to now require the device to
>> flush queued pkts when transitioning to a .can_receive=true state.  Rocker
>> device was not flushing the queue on .can_receive=true transition, so the
>> receiver was stuck.
>>
>> But, turns out we really don't want any queuing at all on the port when the
>> port is disabled, otherwise when the port transitions to enabled, we'd
>> receive and forward stale pkts that really should have been dropped.  So,
>> let's remove .can_receive so avoid queuing and drop the pkt in .receive if
>> the port is disabled.
>>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> ---
>>  hw/net/rocker/rocker_fp.c |   18 ++++++++++--------
>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
>> index d8d934c..ac53c13 100644
>> --- a/hw/net/rocker/rocker_fp.c
>> +++ b/hw/net/rocker/rocker_fp.c
>> @@ -125,18 +125,21 @@ int fp_port_eg(FpPort *port, const struct iovec *iov, int iovcnt)
>>      return ROCKER_OK;
>>  }
>>
>> -static int fp_port_can_receive(NetClientState *nc)
>> -{
>> -    FpPort *port = qemu_get_nic_opaque(nc);
>> -
>> -    return port->enabled;
>> -}
>> -
>>  static ssize_t fp_port_receive_iov(NetClientState *nc, const struct iovec *iov,
>>                                     int iovcnt)
>>  {
>>      FpPort *port = qemu_get_nic_opaque(nc);
>>
>> +    /* If the port is disabled, we want to drop this pkt
>> +     * now rather than queing it for later.  We don't want
>> +     * any stale pkts getting into the device when the port
>> +     * transitions to enabled.
>> +     */
>> +
>> +    if (!port->enabled) {
>> +        return iov_size(iov, iovcnt);
>
> Other devices return -1 when dropping packets, for example see
> e1000_receive_iov or virtio_net_receive. Maybe return -1 here too?

I posted v2 with the change.  Perhaps we should have:

#define QEMU_SEND_PACKET_DROPPED -1

Or something like that...

-scott