Re: [Qemu-devel] [PATCH V11 16/20] filter: Add handle_event method for NetFilterClass

[Zhang Chen]

On Tue, Aug 28, 2018 at 3:19 PM Jason Wang <address@hidden> wrote:

>
>
> On 2018年08月24日 13:57, Zhang Chen wrote:
> > On Wed, Aug 22, 2018 at 4:22 PM Jason Wang<address@hidden>  wrote:
> >
> >> On 2018年08月21日 17:25, Zhang Chen wrote:
> >>> On Tue, Aug 21, 2018 at 11:30 AM Jason Wang<address@hidden>
> wrote:
> >>>
> >>>> On 2018年08月12日 04:59, Zhang Chen wrote:
> >>>>> Filter needs to process the event of checkpoint/failover or
> >>>>> other event passed by COLO frame.
> >>>>>
> >>>>> Signed-off-by: zhanghailiang<address@hidden>
> >>>>> ---
> >>>>>     include/net/filter.h |  5 +++++
> >>>>>     net/filter.c         | 17 +++++++++++++++++
> >>>>>     net/net.c            | 28 ++++++++++++++++++++++++++++
> >>>>>     3 files changed, 50 insertions(+)
> >>>>>
> >>>>> diff --git a/include/net/filter.h b/include/net/filter.h
> >>>>> index 435acd6f82..49da666ac0 100644
> >>>>> --- a/include/net/filter.h
> >>>>> +++ b/include/net/filter.h
> >>>>> @@ -38,6 +38,8 @@ typedef ssize_t (FilterReceiveIOV)(NetFilterState
> >> *nc,
> >>>>>     typedef void (FilterStatusChanged) (NetFilterState *nf, Error
> >> **errp);
> >>>>> +typedef void (FilterHandleEvent) (NetFilterState *nf, int event,
> Error
> >>>> **errp);
> >>>>> +
> >>>>>     typedef struct NetFilterClass {
> >>>>>         ObjectClass parent_class;
> >>>>>
> >>>>> @@ -45,6 +47,7 @@ typedef struct NetFilterClass {
> >>>>>         FilterSetup *setup;
> >>>>>         FilterCleanup *cleanup;
> >>>>>         FilterStatusChanged *status_changed;
> >>>>> +    FilterHandleEvent *handle_event;
> >>>>>         /* mandatory */
> >>>>>         FilterReceiveIOV *receive_iov;
> >>>>>     } NetFilterClass;
> >>>>> @@ -77,4 +80,6 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState
> >>>> *sender,
> >>>>>                                         int iovcnt,
> >>>>>                                         void *opaque);
> >>>>>
> >>>>> +void colo_notify_filters_event(int event, Error **errp);
> >>>>> +
> >>>>>     #endif /* QEMU_NET_FILTER_H */
> >>>>> diff --git a/net/filter.c b/net/filter.c
> >>>>> index 2fd7d7d663..0f17eba143 100644
> >>>>> --- a/net/filter.c
> >>>>> +++ b/net/filter.c
> >>>>> @@ -17,6 +17,8 @@
> >>>>>     #include "net/vhost_net.h"
> >>>>>     #include "qom/object_interfaces.h"
> >>>>>     #include "qemu/iov.h"
> >>>>> +#include "net/colo.h"
> >>>>> +#include "migration/colo.h"
> >>>>>
> >>>>>     static inline bool qemu_can_skip_netfilter(NetFilterState *nf)
> >>>>>     {
> >>>>> @@ -245,11 +247,26 @@ static void netfilter_finalize(Object *obj)
> >>>>>         g_free(nf->netdev_id);
> >>>>>     }
> >>>>>
> >>>>> +static void dummy_handle_event(NetFilterState *nf, int event, Error
> >>>> **errp)
> >>>>> +{
> >>>> It's in fact not a "dummy" handler, Maybe it's better to rename it as
> >>>> "default".
> >>>>
> >>> OK, I will rename it in next version.
> >>>
> >>>
> >>>>> +    switch (event) {
> >>>>> +    case COLO_EVENT_CHECKPOINT:
> >>>>> +        break;
> >>>>> +    case COLO_EVENT_FAILOVER:
> >>>>> +        object_property_set_str(OBJECT(nf), "off", "status", errp);
> >>>> I think filter is a generic infrastructure, so it's better not have
> COLO
> >>>> specific things like this. You can either add a generic name or have a
> >>>> dedicated helper to just disable all net filters.
> >>>>
> >>> Maybe we can rename it to "EVENT_CHECKPOINT" and "EVENT_FAILOVER" looks
> >>> better?
> >>>
> >> I think registering notifier to COLO is better. For disabling filter, we
> >> can have a generic dedicated helpers to do this.
> >>
> > If we registering notifier to COLO (in migration/colo.c), filter-rewriter
> > can not get the notify immediately when checkout occur.
> > filter-reweiter didn't know when checkpoint happened,
>
> I may miss something, isn't COLO which triggers the checkpoint even?
>

Yes, in detail, we have two way to trigger the checkpoint:
case 1. COLO compare trigger.
case 2. periodic checkpoint mechanism trigger.

But we both solve the same problem that how to notify filter-rewriter do
checkpoint in secondary node.
Currently we do this job in that way:

case 1. primary node COLO compare(net/colo-compare.c)  ----> primary node
COLO frame(migration/colo.c) ------> secondary node COLO frame
-----------------> secondary node filter-rewriter.
case 2. primary node COLO frame(migration/colo.c) ------> secondary node
COLO frame -----------------> secondary node filter-rewriter.

So, this patch just focus on "secondary node COLO frame ----------------->
secondary node filter-rewriter".
In filter-rewriter hard to get the checkpoint message from COLO frame.


Thanks
Zhang Chen


>
> >   and I think loop to
> > check the COLO event in filter-rewriter is a bad choice.
>
> Well, anyway you need a loop somewhere to iterate all notifiers?
>
> Thanks
>
> > Any thing I misunderstand?
> >
> >
> >
>
>