[Qemu-devel] [PATCH] net: add drop_packets parameter to -net nic

[Qemu-devel] [PATCH] net: add drop_packets parameter to -net nic

[Nguyễn Thái Ngọc Duy]

Dropping packets is sometimes perferred behavior. Add drop_packets
parameter to NICConf struct and let nic simulation decide how to use
it.

Only e1000 supports this for now.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation is missing, but I'm not even sure if there's any other
 user who finds this useful.

 hw/e1000.c |    4 +++-
 hw/qdev.c  |    1 +
 net.c      |    5 +++++
 net.h      |    9 +++++++--
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index fe3e812..57ffdec 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -656,7 +656,9 @@ e1000_can_receive(VLANClientState *nc)
 {
     E1000State *s = DO_UPCAST(NICState, nc, nc)->opaque;
 
-    return (s->mac_reg[RCTL] & E1000_RCTL_EN) && e1000_has_rxbufs(s, 1);
+    return (s->conf.flags & NIC_CONF_DROP_PACKETS) ||
+           ((s->mac_reg[RCTL] & E1000_RCTL_EN) &&
+            e1000_has_rxbufs(s, 1));
 }
 
 static uint64_t rx_desc_base(E1000State *s)
diff --git a/hw/qdev.c b/hw/qdev.c
index 9519f5d..d8605d6 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -468,6 +468,7 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
         qdev_prop_exists(dev, "vectors")) {
         qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
     }
+    qdev_prop_set_bit(dev, "drop_packets", nd->drop_packets);
 }
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
diff --git a/net.c b/net.c
index 6746bc7..566be48 100644
--- a/net.c
+++ b/net.c
@@ -798,6 +798,7 @@ static int net_init_nic(QemuOpts *opts,
         return -1;
     }
 
+    nd->drop_packets = qemu_opt_get_bool(opts, "drop_packets", 0);
     nd->used = 1;
     nb_nics++;
 
@@ -864,6 +865,10 @@ static const struct {
                 .name = "vectors",
                 .type = QEMU_OPT_NUMBER,
                 .help = "number of MSI-x vectors, 0 to disable MSI-X",
+            }, {
+                .name = "drop_packets",
+                .type = QEMU_OPT_BOOL,
+                .help = "drop packets if driver is not ready to receive"
             },
             { /* end of list */ }
         },
diff --git a/net.h b/net.h
index 6ceca50..a594313 100644
--- a/net.h
+++ b/net.h
@@ -12,19 +12,23 @@ struct MACAddr {
 };
 
 /* qdev nic properties */
+#define NIC_CONF_DROP_PACKETS_BIT   0
+#define NIC_CONF_DROP_PACKETS      (1 << NIC_CONF_DROP_PACKETS_BIT)
 
 typedef struct NICConf {
     MACAddr macaddr;
     VLANState *vlan;
     VLANClientState *peer;
     int32_t bootindex;
+    uint32_t flags;
 } NICConf;
 
 #define DEFINE_NIC_PROPERTIES(_state, _conf)                            \
     DEFINE_PROP_MACADDR("mac",   _state, _conf.macaddr),                \
     DEFINE_PROP_VLAN("vlan",     _state, _conf.vlan),                   \
     DEFINE_PROP_NETDEV("netdev", _state, _conf.peer),                   \
-    DEFINE_PROP_INT32("bootindex", _state, _conf.bootindex, -1)
+    DEFINE_PROP_INT32("bootindex", _state, _conf.bootindex, -1),        \
+    DEFINE_PROP_BIT("drop_packets", _state, _conf.flags, NIC_CONF_DROP_PACKETS_BIT, 0)
 
 /* VLANs support */
 
@@ -133,8 +137,9 @@ struct NICInfo {
     char *devaddr;
     VLANState *vlan;
     VLANClientState *netdev;
-    int used;
     int nvectors;
+    unsigned int used : 1;
+    unsigned int drop_packets : 1;
 };
 
 extern int nb_nics;
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH] net: add drop_packets parameter to -net nic

[Stefan Hajnoczi]

2011/4/25 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> Dropping packets is sometimes perferred behavior. Add drop_packets
> parameter to NICConf struct and let nic simulation decide how to use
> it.
>
> Only e1000 supports this for now.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation is missing, but I'm not even sure if there's any other
>  user who finds this useful.

Can you explain why you are adding this?  You are trying to bypass the
send queue and drop packets instead?

Stefan

Re: [Qemu-devel] [PATCH] net: add drop_packets parameter to -net nic

[Anthony Liguori]

On 04/25/2011 08:40 AM, Stefan Hajnoczi wrote:
> 2011/4/25 Nguyễn Thái Ngọc Duy<pclouds@gmail.com>:
>> Dropping packets is sometimes perferred behavior. Add drop_packets
>> parameter to NICConf struct and let nic simulation decide how to use
>> it.
>>
>> Only e1000 supports this for now.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy<pclouds@gmail.com>
>> ---
>>   Documentation is missing, but I'm not even sure if there's any other
>>   user who finds this useful.
>
> Can you explain why you are adding this?  You are trying to bypass the
> send queue and drop packets instead?

And some performance results always help with this sort of thing.

Regards,

Anthony Liguori

>
> Stefan
>

Re: [Qemu-devel] [PATCH] net: add drop_packets parameter to -net nic

[Nguyen Thai Ngoc Duy]

2011/4/25 Stefan Hajnoczi <stefanha@gmail.com>:
> 2011/4/25 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>> Dropping packets is sometimes perferred behavior. Add drop_packets
>> parameter to NICConf struct and let nic simulation decide how to use
>> it.
>>
>> Only e1000 supports this for now.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  Documentation is missing, but I'm not even sure if there's any other
>>  user who finds this useful.
>
> Can you explain why you are adding this?  You are trying to bypass the
> send queue and drop packets instead?

Yes.

I have a driver that does connection hand shaking at ethernet level.
If anything goes wrong, it disables rx and after a while a new session
will be started from higher level. The other end has a timer and keeps
sending data until it times out. If this end does not respond properly
until the timer is timed out, the other end starts sending "connection
request" packets periodically for a new session. When this end enables
rx again, in real world it would receive a fresh req packet and send
ack. Because of queuing, it receives packets from old session and
sends out a series of nack because it expects req packet. Depends on
how long rx is disabled until a new session is started, the driver
will have to process all queued (invalid) packets and delay session
establishment some more.

I think dropping packets will improve this situation. But again, this
driver is peculiar. I don't think there are many drivers that do
dialog-style like this.
-- 
Duy

Re: [Qemu-devel] [PATCH] net: add drop_packets parameter to -net nic

[Stefan Hajnoczi]

On Mon, Apr 25, 2011 at 3:06 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> 2011/4/25 Stefan Hajnoczi <stefanha@gmail.com>:
>> 2011/4/25 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>>> Dropping packets is sometimes perferred behavior. Add drop_packets
>>> parameter to NICConf struct and let nic simulation decide how to use
>>> it.
>>>
>>> Only e1000 supports this for now.
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>>  Documentation is missing, but I'm not even sure if there's any other
>>>  user who finds this useful.
>>
>> Can you explain why you are adding this?  You are trying to bypass the
>> send queue and drop packets instead?
>
> Yes.
>
> I have a driver that does connection hand shaking at ethernet level.
> If anything goes wrong, it disables rx and after a while a new session
> will be started from higher level. The other end has a timer and keeps
> sending data until it times out. If this end does not respond properly
> until the timer is timed out, the other end starts sending "connection
> request" packets periodically for a new session. When this end enables
> rx again, in real world it would receive a fresh req packet and send
> ack. Because of queuing, it receives packets from old session and
> sends out a series of nack because it expects req packet. Depends on
> how long rx is disabled until a new session is started, the driver
> will have to process all queued (invalid) packets and delay session
> establishment some more.
>
> I think dropping packets will improve this situation. But again, this
> driver is peculiar. I don't think there are many drivers that do
> dialog-style like this.

The behavior you are describing sounds like a bug in QEMU's network
layer.  If RX is disabled we should not queue incoming packets.

Have you looked into fixing QEMU so that the queue is disabled when RX
is disabled?

Stefan

Re: [Qemu-devel] [PATCH] net: add drop_packets parameter to -net nic

[Nguyen Thai Ngoc Duy]

On Tue, Apr 26, 2011 at 4:14 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> The behavior you are describing sounds like a bug in QEMU's network
> layer.  If RX is disabled we should not queue incoming packets.
>
> Have you looked into fixing QEMU so that the queue is disabled when RX
> is disabled?

it's in e1000_can_receive(): it can receive if rx is enabled
(E1000_RCTL_EN) and has enough buffer, which means if the driver
disables rx, packets queue up. Isn't that correct behavior? Sorry I'm
new in this area.
-- 
Duy