Re: [Qemu-devel] [PATCH v4 1/5] net/announce: Allow optional list of interfaces

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Jason Wang (jasowang@redhat.com) wrote:
> 
> On 2019/6/13 下午5:59, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Allow the caller to restrict the set of interfaces that announces are
> > sent on.  The default is still to send on all interfaces.
> > 
> > e.g.
> > 
> >    { "execute": "announce-self", "arguments": { "initial": 50, "max": 550, "rounds": 5, "step": 50, "interfaces": ["vn2", "vn1"] } }
> > 
> > This doesn't affect the behaviour of migraiton announcments.
> > 
> > Note: There's still only one timer for the qmp command, so that
> > performing an 'announce-self' on one list of interfaces followed
> > by another 'announce-self' on another list will stop the announces
> > on the existing set.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >   include/net/announce.h |  2 +-
> >   net/announce.c         | 39 ++++++++++++++++++++++++++++++++-------
> >   net/trace-events       |  2 +-
> >   qapi/net.json          | 11 ++++++++---
> >   4 files changed, 42 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/net/announce.h b/include/net/announce.h
> > index 04a035f679..773470428b 100644
> > --- a/include/net/announce.h
> > +++ b/include/net/announce.h
> > @@ -22,7 +22,7 @@ struct AnnounceTimer {
> >   /* Returns: update the timer to the next time point */
> >   int64_t qemu_announce_timer_step(AnnounceTimer *timer);
> > -/* Delete the underlying timer */
> > +/* Delete the underlying timer and other data */
> >   void qemu_announce_timer_del(AnnounceTimer *timer);
> >   /*
> > diff --git a/net/announce.c b/net/announce.c
> > index 91e9a6e267..1ce42b571d 100644
> > --- a/net/announce.c
> > +++ b/net/announce.c
> > @@ -38,6 +38,8 @@ void qemu_announce_timer_del(AnnounceTimer *timer)
> >           timer_free(timer->tm);
> >           timer->tm = NULL;
> >       }
> > +    qapi_free_strList(timer->params.interfaces);
> > +    timer->params.interfaces = NULL;
> >   }
> >   /*
> > @@ -96,24 +98,47 @@ static int announce_self_create(uint8_t *buf,
> >   static void qemu_announce_self_iter(NICState *nic, void *opaque)
> >   {
> > +    AnnounceTimer *timer = opaque;
> >       uint8_t buf[60];
> >       int len;
> > +    bool skip;
> > +
> > +    if (timer->params.has_interfaces) {
> > +        strList *entry = timer->params.interfaces;
> > +        /* Skip unless we find our name in the requested list */
> > +        skip = true;
> > +
> > +        while (entry) {
> > +            if (!strcmp(entry->value, nic->ncs->name)) {
> > +                /* Found us */
> > +                skip = false;
> > +                break;
> > +            }
> > +            entry = entry->next;
> > +        }
> > +    } else {
> > +        skip = false;
> > +    }
> 
> 
> I wonder whether or not it's better to filter the name on the caller.

Doing it this way means I don't have to worry about any hotplug that
might happen during the announce period.

Dave

> Thanks
> 
> 
> > +
> > +    trace_qemu_announce_self_iter(nic->ncs->name,
> > +                                  qemu_ether_ntoa(&nic->conf->macaddr), skip);
> > -    trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> > -    len = announce_self_create(buf, nic->conf->macaddr.a);
> > +    if (!skip) {
> > +        len = announce_self_create(buf, nic->conf->macaddr.a);
> > -    qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> > +        qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> > -    /* if the NIC provides it's own announcement support, use it as well */
> > -    if (nic->ncs->info->announce) {
> > -        nic->ncs->info->announce(nic->ncs);
> > +        /* if the NIC provides it's own announcement support, use it as well */
> > +        if (nic->ncs->info->announce) {
> > +            nic->ncs->info->announce(nic->ncs);
> > +        }
> >       }
> >   }
> >   static void qemu_announce_self_once(void *opaque)
> >   {
> >       AnnounceTimer *timer = (AnnounceTimer *)opaque;
> > -    qemu_foreach_nic(qemu_announce_self_iter, NULL);
> > +    qemu_foreach_nic(qemu_announce_self_iter, timer);
> >       if (--timer->round) {
> >           qemu_announce_timer_step(timer);
> > diff --git a/net/trace-events b/net/trace-events
> > index a7937f3f3a..875ef2a0f3 100644
> > --- a/net/trace-events
> > +++ b/net/trace-events
> > @@ -1,7 +1,7 @@
> >   # See docs/devel/tracing.txt for syntax documentation.
> >   # announce.c
> > -qemu_announce_self_iter(const char *mac) "%s"
> > +qemu_announce_self_iter(const char *name, const char *mac, int skip) "%s:%s skip: %d"
> >   # vhost-user.c
> >   vhost_user_event(const char *chr, int event) "chr: %s got event: %d"
> > diff --git a/qapi/net.json b/qapi/net.json
> > index 5f7bff1637..6f2cd4f530 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -699,6 +699,9 @@
> >   #
> >   # @step: Delay increase (in ms) after each self-announcement attempt
> >   #
> > +# @interfaces: An optional list of interface names, which restricts the
> > +#        announcement to the listed interfaces. (Since 4.1)
> > +#
> >   # Since: 4.0
> >   ##
> > @@ -706,7 +709,8 @@
> >     'data': { 'initial': 'int',
> >               'max': 'int',
> >               'rounds': 'int',
> > -            'step': 'int' } }
> > +            'step': 'int',
> > +            '*interfaces': ['str'] } }
> >   ##
> >   # @announce-self:
> > @@ -718,9 +722,10 @@
> >   #
> >   # Example:
> >   #
> > -# -> { "execute": "announce-self"
> > +# -> { "execute": "announce-self",
> >   #      "arguments": {
> > -#          "initial": 50, "max": 550, "rounds": 10, "step": 50 } }
> > +#          "initial": 50, "max": 550, "rounds": 10, "step": 50,
> > +#          "interfaces": ["vn2", "vn3"] } }
> >   # <- { "return": {} }
> >   #
> >   # Since: 4.0
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK