Re: [Qemu-devel] [PATCH V4 RESEND 09/22] net: multiqueue support

[Alexander Graf]

On 01.02.2013, at 08:39, Jason Wang wrote:

> This patch adds basic multiqueue support for qemu. The idea is simple, an 
> array
> of NetClientStates were introduced in NICState, parse_netdev() were extended 
> to
> find and match all NetClientStates belongs to the backend and place their
> pointers in NICConf. Then qemu_new_nic can setup a N:N mapping between 
> NICStates
> that belongs to a nic and NICStates belongs to the netdev. And a queue_index
> were introduced in NetClientState to track its index. After this, each peers 
> of
> a NICState were abstracted as a queue.
> 
> After this change, all NetClientState that belongs to the same backend/nic has
> the same id. When use want to change the link status, all NetClientStates that
> belongs to the same backend/nic will be also changed. When user want to delete
> a device or netdev, all NetClientStates that belongs to the same backend/nic
> will be deleted also. Changing or deleting an specific queue is not allowed.
> 
> Signed-off-by: Jason Wang <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
> hw/dp8393x.c                |    2 +-
> hw/mcf_fec.c                |    2 +-
> hw/qdev-properties-system.c |   46 +++++++++++++++---
> hw/qdev-properties.h        |    6 +-
> include/net/net.h           |   18 +++++--
> net/net.c                   |  113 +++++++++++++++++++++++++++++++------------
> 6 files changed, 139 insertions(+), 48 deletions(-)
> 
> diff --git a/hw/dp8393x.c b/hw/dp8393x.c
> index 0273fad..808157b 100644
> --- a/hw/dp8393x.c
> +++ b/hw/dp8393x.c
> @@ -900,7 +900,7 @@ void dp83932_init(NICInfo *nd, hwaddr base, int it_shift,
>     s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
> 
>     s->conf.macaddr = nd->macaddr;
> -    s->conf.peer = nd->netdev;
> +    s->conf.peers.ncs[0] = nd->netdev;
> 
>     s->nic = qemu_new_nic(&net_dp83932_info, &s->conf, nd->model, nd->name, 
> s);
> 
> diff --git a/hw/mcf_fec.c b/hw/mcf_fec.c
> index 909e32b..8e60f09 100644
> --- a/hw/mcf_fec.c
> +++ b/hw/mcf_fec.c
> @@ -472,7 +472,7 @@ void mcf_fec_init(MemoryRegion *sysmem, NICInfo *nd,
>     memory_region_add_subregion(sysmem, base, &s->iomem);
> 
>     s->conf.macaddr = nd->macaddr;
> -    s->conf.peer = nd->netdev;
> +    s->conf.peers.ncs[0] = nd->netdev;
> 
>     s->nic = qemu_new_nic(&net_mcf_fec_info, &s->conf, nd->model, nd->name, 
> s);
> 
> diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
> index ce0f793..ce3af22 100644
> --- a/hw/qdev-properties-system.c
> +++ b/hw/qdev-properties-system.c
> @@ -173,16 +173,47 @@ PropertyInfo qdev_prop_chr = {
> 
> static int parse_netdev(DeviceState *dev, const char *str, void **ptr)
> {
> -    NetClientState *netdev = qemu_find_netdev(str);
> +    NICPeers *peers_ptr = (NICPeers *)ptr;
> +    NICConf *conf = container_of(peers_ptr, NICConf, peers);
> +    NetClientState **ncs = peers_ptr->ncs;
> +    NetClientState *peers[MAX_QUEUE_NUM];
> +    int queues, i = 0;
> +    int ret;
> 
> -    if (netdev == NULL) {
> -        return -ENOENT;
> +    queues = qemu_find_net_clients_except(str, peers,
> +                                          NET_CLIENT_OPTIONS_KIND_NIC,
> +                                          MAX_QUEUE_NUM);
> +    if (queues == 0) {
> +        ret = -ENOENT;
> +        goto err;
>     }
> -    if (netdev->peer) {
> -        return -EEXIST;
> +
> +    if (queues > MAX_QUEUE_NUM) {
> +        ret = -E2BIG;
> +        goto err;
> +    }
> +
> +    for (i = 0; i < queues; i++) {
> +        if (peers[i] == NULL) {
> +            ret = -ENOENT;
> +            goto err;
> +        }
> +
> +        if (peers[i]->peer) {
> +            ret = -EEXIST;
> +            goto err;
> +        }
> +
> +        ncs[i] = peers[i];
> +        ncs[i]->queue_index = i;
>     }
> -    *ptr = netdev;
> +
> +    conf->queues = queues;
> +
>     return 0;
> +
> +err:
> +    return ret;
> }
> 
> static const char *print_netdev(void *ptr)
> @@ -249,7 +280,8 @@ static void set_vlan(Object *obj, Visitor *v, void 
> *opaque,
> {
>     DeviceState *dev = DEVICE(obj);
>     Property *prop = opaque;
> -    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> +    NICPeers *peers_ptr = qdev_get_prop_ptr(dev, prop);
> +    NetClientState **ptr = &peers_ptr->ncs[0];
>     Error *local_err = NULL;
>     int32_t id;
>     NetClientState *hubport;
> diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
> index ddcf774..20c67f3 100644
> --- a/hw/qdev-properties.h
> +++ b/hw/qdev-properties.h
> @@ -31,7 +31,7 @@ extern PropertyInfo qdev_prop_pci_host_devaddr;
>         .name      = (_name),                                    \
>         .info      = &(_prop),                                   \
>         .offset    = offsetof(_state, _field)                    \
> -            + type_check(_type,typeof_field(_state, _field)),    \
> +            + type_check(_type, typeof_field(_state, _field)),   \
>         }
> #define DEFINE_PROP_DEFAULT(_name, _state, _field, _defval, _prop, _type) { \
>         .name      = (_name),                                           \
> @@ -77,9 +77,9 @@ extern PropertyInfo qdev_prop_pci_host_devaddr;
> #define DEFINE_PROP_STRING(_n, _s, _f)             \
>     DEFINE_PROP(_n, _s, _f, qdev_prop_string, char*)
> #define DEFINE_PROP_NETDEV(_n, _s, _f)             \
> -    DEFINE_PROP(_n, _s, _f, qdev_prop_netdev, NetClientState*)
> +    DEFINE_PROP(_n, _s, _f, qdev_prop_netdev, NICPeers)
> #define DEFINE_PROP_VLAN(_n, _s, _f)             \
> -    DEFINE_PROP(_n, _s, _f, qdev_prop_vlan, NetClientState*)
> +    DEFINE_PROP(_n, _s, _f, qdev_prop_vlan, NICPeers)
> #define DEFINE_PROP_DRIVE(_n, _s, _f) \
>     DEFINE_PROP(_n, _s, _f, qdev_prop_drive, BlockDriverState *)
> #define DEFINE_PROP_MACADDR(_n, _s, _f)         \
> diff --git a/include/net/net.h b/include/net/net.h
> index 22adc99..43a045e 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -9,24 +9,32 @@
> #include "migration/vmstate.h"
> #include "qapi-types.h"
> 
> +#define MAX_QUEUE_NUM 1024
> +
> struct MACAddr {
>     uint8_t a[6];
> };
> 
> /* qdev nic properties */
> 
> +typedef struct NICPeers {
> +    NetClientState *ncs[MAX_QUEUE_NUM];
> +} NICPeers;
> +
> typedef struct NICConf {
>     MACAddr macaddr;
> -    NetClientState *peer;
> +    NICPeers peers;
>     int32_t bootindex;
> +    int32_t queues;
> } NICConf;
> 
> #define DEFINE_NIC_PROPERTIES(_state, _conf)                            \
>     DEFINE_PROP_MACADDR("mac",   _state, _conf.macaddr),                \
> -    DEFINE_PROP_VLAN("vlan",     _state, _conf.peer),                   \
> -    DEFINE_PROP_NETDEV("netdev", _state, _conf.peer),                   \
> +    DEFINE_PROP_VLAN("vlan",     _state, _conf.peers),                   \
> +    DEFINE_PROP_NETDEV("netdev", _state, _conf.peers),                   \
>     DEFINE_PROP_INT32("bootindex", _state, _conf.bootindex, -1)
> 
> +
> /* Net clients */
> 
> typedef void (NetPoll)(NetClientState *, bool enable);
> @@ -60,10 +68,11 @@ struct NetClientState {
>     char info_str[256];
>     unsigned receive_disabled : 1;
>     NetClientDestructor *destructor;
> +    unsigned int queue_index;
> };
> 
> typedef struct NICState {
> -    NetClientState nc;
> +    NetClientState ncs[MAX_QUEUE_NUM];
>     NICConf *conf;
>     void *opaque;
>     bool peer_deleted;
> @@ -82,6 +91,7 @@ NICState *qemu_new_nic(NetClientInfo *info,
>                        const char *name,
>                        void *opaque);
> void qemu_del_nic(NICState *nic);
> +NetClientState *qemu_get_subqueue(NICState *nic, int queue_index);
> NetClientState *qemu_get_queue(NICState *nic);
> NICState *qemu_get_nic(NetClientState *nc);
> void *qemu_get_nic_opaque(NetClientState *nc);
> diff --git a/net/net.c b/net/net.c
> index 98a1934..9806862 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -236,28 +236,44 @@ NICState *qemu_new_nic(NetClientInfo *info,
>                        void *opaque)
> {
>     NetClientState *nc;
> +    NetClientState **peers = conf->peers.ncs;
>     NICState *nic;
> +    int i;
> 
>     assert(info->type == NET_CLIENT_OPTIONS_KIND_NIC);
>     assert(info->size >= sizeof(NICState));
> 
> -    nc = qemu_new_net_client(info, conf->peer, model, name);
> +    nc = qemu_new_net_client(info, peers[0], model, name);
> +    nc->queue_index = 0;
> 
>     nic = qemu_get_nic(nc);
>     nic->conf = conf;
>     nic->opaque = opaque;
> 
> +    for (i = 1; i < conf->queues; i++) {
> +        qemu_net_client_setup(&nic->ncs[i], info, peers[i], model, nc->name,
> +                              NULL);
> +        nic->ncs[i].queue_index = i;
> +    }
> +
>     return nic;
> }
> 
> +NetClientState *qemu_get_subqueue(NICState *nic, int queue_index)
> +{
> +    return &nic->ncs[queue_index];
> +}
> +
> NetClientState *qemu_get_queue(NICState *nic)
> {
> -    return &nic->nc;
> +    return qemu_get_subqueue(nic, 0);
> }
> 
> NICState *qemu_get_nic(NetClientState *nc)
> {
> -    return DO_UPCAST(NICState, nc, nc);
> +    NetClientState *nc0 = nc - nc->queue_index;
> +
> +    return DO_UPCAST(NICState, ncs[0], nc0);
> }
> 
> void *qemu_get_nic_opaque(NetClientState *nc)
> @@ -271,9 +287,7 @@ static void qemu_cleanup_net_client(NetClientState *nc)
> {
>     QTAILQ_REMOVE(&net_clients, nc, next);
> 
> -    if (nc->info->cleanup) {
> -        nc->info->cleanup(nc);
> -    }
> +    nc->info->cleanup(nc);

This breaks the following backends:

hw/pcnet.c
hw/spapr_llan.c
hw/xen_nic.c

Is this change on purpose? If so, please provide empty cleanup function 
callbacks for the above targets.


Alex