| Message ID | 1454750932-7556-38-git-send-email-zhang.zhanghailiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 02/06/2016 05:28 PM, zhanghailiang wrote: > Enable all buffer filters that added by COLO while > go into COLO process, and disable them while exit COLO. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Yang Hongyang <hongyang.yang@easystack.cn> > --- > v14: > - New patch > --- > migration/colo.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/migration/colo.c b/migration/colo.c > index 235578b..86a7638 100644 > --- a/migration/colo.c > +++ b/migration/colo.c > @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void) > } > } > > +static void colo_set_filter_status(NetFilterState *nf, void *opaque, > + Error **errp) > +{ > + char colo_filter[128]; > + char *name = object_get_canonical_path_component(OBJECT(nf)); > + char *status = opaque; > + > + snprintf(colo_filter, sizeof(colo_filter), "%scolo", nf->netdev_id); > + if (strcmp(colo_filter, name)) { > + return; > + } Checking by name is not elegant. As we've discussed last time, why not let filter-buffer track all filters with zero interval in a linked list and just export a helper to disable and enable them all? Things will be greatly simplified with this, and there's even no need for patch 36.
On 2016/2/18 11:31, Jason Wang wrote: > > > On 02/06/2016 05:28 PM, zhanghailiang wrote: >> Enable all buffer filters that added by COLO while >> go into COLO process, and disable them while exit COLO. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Cc: Jason Wang <jasowang@redhat.com> >> Cc: Yang Hongyang <hongyang.yang@easystack.cn> >> --- >> v14: >> - New patch >> --- >> migration/colo.c | 32 ++++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/migration/colo.c b/migration/colo.c >> index 235578b..86a7638 100644 >> --- a/migration/colo.c >> +++ b/migration/colo.c >> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void) >> } >> } >> >> +static void colo_set_filter_status(NetFilterState *nf, void *opaque, >> + Error **errp) >> +{ >> + char colo_filter[128]; >> + char *name = object_get_canonical_path_component(OBJECT(nf)); >> + char *status = opaque; >> + >> + snprintf(colo_filter, sizeof(colo_filter), "%scolo", nf->netdev_id); >> + if (strcmp(colo_filter, name)) { >> + return; >> + } > > Checking by name is not elegant. As we've discussed last time, why not > let filter-buffer track all filters with zero interval in a linked list > and just export a helper to disable and enable them all? Things will be > greatly simplified with this, and there's even no need for patch 36. > > Yes, i know what you mean, but we have to add another 'QTAILQ_ENTRY() entry' like member into struct NetFilterState if we do like that, is it acceptable ? > . >
Hi Jason, On 2016/2/18 11:46, Hailiang Zhang wrote: > On 2016/2/18 11:31, Jason Wang wrote: >> >> >> On 02/06/2016 05:28 PM, zhanghailiang wrote: >>> Enable all buffer filters that added by COLO while >>> go into COLO process, and disable them while exit COLO. >>> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>> Cc: Jason Wang <jasowang@redhat.com> >>> Cc: Yang Hongyang <hongyang.yang@easystack.cn> >>> --- >>> v14: >>> - New patch >>> --- >>> migration/colo.c | 32 ++++++++++++++++++++++++++++++++ >>> 1 file changed, 32 insertions(+) >>> >>> diff --git a/migration/colo.c b/migration/colo.c >>> index 235578b..86a7638 100644 >>> --- a/migration/colo.c >>> +++ b/migration/colo.c >>> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void) >>> } >>> } >>> >>> +static void colo_set_filter_status(NetFilterState *nf, void *opaque, >>> + Error **errp) >>> +{ >>> + char colo_filter[128]; >>> + char *name = object_get_canonical_path_component(OBJECT(nf)); >>> + char *status = opaque; >>> + >>> + snprintf(colo_filter, sizeof(colo_filter), "%scolo", nf->netdev_id); >>> + if (strcmp(colo_filter, name)) { >>> + return; >>> + } >> >> Checking by name is not elegant. As we've discussed last time, why not >> let filter-buffer track all filters with zero interval in a linked list >> and just export a helper to disable and enable them all? Things will be >> greatly simplified with this, and there's even no need for patch 36. >> >> > > Yes, i know what you mean, but we have to add another > 'QTAILQ_ENTRY() entry' like member into struct NetFilterState > if we do like that, is it acceptable ? > Sorry for the hasty reply, ;) We can use a list to store all the colo related buffer filters without adding any member to struct NetFilterState. The codes will be like: struct COLOListNode{ void *opaque; QLIST_ENTRY(COLOListNode) node; }; static QLIST_HEAD(, COLOListNode) COLOBufferFilters = QLIST_HEAD_INITIALIZER(COLOBufferFilters); void colo_add_buffer_filter() { struct COLOListNode *filternode; ... filter = object_new_with_props(); filternode = g_new0(struct COLOListNode, 1); filternode->opaque = NETFILTER(filter); QLIST_INSERT_HEAD(&COLOBufferFilters, filternode, node); } And we can track all the colo releated filters directly by visiting the *COLOBufferFilters* list. Thanks, Hailiang >> . >> >
On 02/18/2016 11:46 AM, Hailiang Zhang wrote: > On 2016/2/18 11:31, Jason Wang wrote: >> >> >> On 02/06/2016 05:28 PM, zhanghailiang wrote: >>> Enable all buffer filters that added by COLO while >>> go into COLO process, and disable them while exit COLO. >>> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>> Cc: Jason Wang <jasowang@redhat.com> >>> Cc: Yang Hongyang <hongyang.yang@easystack.cn> >>> --- >>> v14: >>> - New patch >>> --- >>> migration/colo.c | 32 ++++++++++++++++++++++++++++++++ >>> 1 file changed, 32 insertions(+) >>> >>> diff --git a/migration/colo.c b/migration/colo.c >>> index 235578b..86a7638 100644 >>> --- a/migration/colo.c >>> +++ b/migration/colo.c >>> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void) >>> } >>> } >>> >>> +static void colo_set_filter_status(NetFilterState *nf, void *opaque, >>> + Error **errp) >>> +{ >>> + char colo_filter[128]; >>> + char *name = object_get_canonical_path_component(OBJECT(nf)); >>> + char *status = opaque; >>> + >>> + snprintf(colo_filter, sizeof(colo_filter), "%scolo", >>> nf->netdev_id); >>> + if (strcmp(colo_filter, name)) { >>> + return; >>> + } >> >> Checking by name is not elegant. As we've discussed last time, why not >> let filter-buffer track all filters with zero interval in a linked list >> and just export a helper to disable and enable them all? Things will be >> greatly simplified with this, and there's even no need for patch 36. >> >> > > Yes, i know what you mean, but we have to add another > 'QTAILQ_ENTRY() entry' like member into struct NetFilterState > if we do like that, is it acceptable ? I think you can make it private to filter buffer itself.
On 02/18/2016 03:30 PM, Hailiang Zhang wrote: > Hi Jason, > > On 2016/2/18 11:46, Hailiang Zhang wrote: >> On 2016/2/18 11:31, Jason Wang wrote: >>> >>> >>> On 02/06/2016 05:28 PM, zhanghailiang wrote: >>>> Enable all buffer filters that added by COLO while >>>> go into COLO process, and disable them while exit COLO. >>>> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>> Cc: Jason Wang <jasowang@redhat.com> >>>> Cc: Yang Hongyang <hongyang.yang@easystack.cn> >>>> --- >>>> v14: >>>> - New patch >>>> --- >>>> migration/colo.c | 32 ++++++++++++++++++++++++++++++++ >>>> 1 file changed, 32 insertions(+) >>>> >>>> diff --git a/migration/colo.c b/migration/colo.c >>>> index 235578b..86a7638 100644 >>>> --- a/migration/colo.c >>>> +++ b/migration/colo.c >>>> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void) >>>> } >>>> } >>>> >>>> +static void colo_set_filter_status(NetFilterState *nf, void *opaque, >>>> + Error **errp) >>>> +{ >>>> + char colo_filter[128]; >>>> + char *name = object_get_canonical_path_component(OBJECT(nf)); >>>> + char *status = opaque; >>>> + >>>> + snprintf(colo_filter, sizeof(colo_filter), "%scolo", >>>> nf->netdev_id); >>>> + if (strcmp(colo_filter, name)) { >>>> + return; >>>> + } >>> >>> Checking by name is not elegant. As we've discussed last time, why not >>> let filter-buffer track all filters with zero interval in a linked list >>> and just export a helper to disable and enable them all? Things will be >>> greatly simplified with this, and there's even no need for patch 36. >>> >>> >> >> Yes, i know what you mean, but we have to add another >> 'QTAILQ_ENTRY() entry' like member into struct NetFilterState >> if we do like that, is it acceptable ? >> > > Sorry for the hasty reply, ;) > > We can use a list to store all the colo related buffer filters > without adding any member to struct NetFilterState. > > The codes will be like: > > struct COLOListNode{ > void *opaque; > QLIST_ENTRY(COLOListNode) node; > }; > static QLIST_HEAD(, COLOListNode) COLOBufferFilters = > QLIST_HEAD_INITIALIZER(COLOBufferFilters); > > void colo_add_buffer_filter() > { > struct COLOListNode *filternode; > ... > > filter = object_new_with_props(); > > filternode = g_new0(struct COLOListNode, 1); > filternode->opaque = NETFILTER(filter); > QLIST_INSERT_HEAD(&COLOBufferFilters, filternode, node); > } > > And we can track all the colo releated filters directly by > visiting the *COLOBufferFilters* list. > > Thanks, > Hailiang Also fine, but looks suboptimal to my proposal (use a list private to filter-buffer). > >>> . >>> >> > > >
Hi Jason, Thanks for your patience. On 2016/2/23 16:38, Jason Wang wrote: > > > On 02/18/2016 03:30 PM, Hailiang Zhang wrote: >> Hi Jason, >> >> On 2016/2/18 11:46, Hailiang Zhang wrote: >>> On 2016/2/18 11:31, Jason Wang wrote: >>>> >>>> >>>> On 02/06/2016 05:28 PM, zhanghailiang wrote: >>>>> Enable all buffer filters that added by COLO while >>>>> go into COLO process, and disable them while exit COLO. >>>>> >>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>>> Cc: Jason Wang <jasowang@redhat.com> >>>>> Cc: Yang Hongyang <hongyang.yang@easystack.cn> >>>>> --- >>>>> v14: >>>>> - New patch >>>>> --- >>>>> migration/colo.c | 32 ++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 32 insertions(+) >>>>> >>>>> diff --git a/migration/colo.c b/migration/colo.c >>>>> index 235578b..86a7638 100644 >>>>> --- a/migration/colo.c >>>>> +++ b/migration/colo.c >>>>> @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void) >>>>> } >>>>> } >>>>> >>>>> +static void colo_set_filter_status(NetFilterState *nf, void *opaque, >>>>> + Error **errp) >>>>> +{ >>>>> + char colo_filter[128]; >>>>> + char *name = object_get_canonical_path_component(OBJECT(nf)); >>>>> + char *status = opaque; >>>>> + >>>>> + snprintf(colo_filter, sizeof(colo_filter), "%scolo", >>>>> nf->netdev_id); >>>>> + if (strcmp(colo_filter, name)) { >>>>> + return; >>>>> + } >>>> >>>> Checking by name is not elegant. As we've discussed last time, why not >>>> let filter-buffer track all filters with zero interval in a linked list >>>> and just export a helper to disable and enable them all? Things will be >>>> greatly simplified with this, and there's even no need for patch 36. >>>> >>>> >>> >>> Yes, i know what you mean, but we have to add another >>> 'QTAILQ_ENTRY() entry' like member into struct NetFilterState >>> if we do like that, is it acceptable ? >>> >> >> Sorry for the hasty reply, ;) >> >> We can use a list to store all the colo related buffer filters >> without adding any member to struct NetFilterState. >> >> The codes will be like: >> >> struct COLOListNode{ >> void *opaque; >> QLIST_ENTRY(COLOListNode) node; >> }; >> static QLIST_HEAD(, COLOListNode) COLOBufferFilters = >> QLIST_HEAD_INITIALIZER(COLOBufferFilters); >> >> void colo_add_buffer_filter() >> { >> struct COLOListNode *filternode; >> ... >> >> filter = object_new_with_props(); >> >> filternode = g_new0(struct COLOListNode, 1); >> filternode->opaque = NETFILTER(filter); >> QLIST_INSERT_HEAD(&COLOBufferFilters, filternode, node); >> } >> >> And we can track all the colo releated filters directly by >> visiting the *COLOBufferFilters* list. >> >> Thanks, >> Hailiang > I have updated these codes in 'PATCH COLO-Frame v15', which is sent yesterday. :) > Also fine, but looks suboptimal to my proposal (use a list private to > filter-buffer). > Yes, i have consider adding such a private member to filter-buffer, but it seems to be only used by COLO, which looks strange, and we tried to avoid dirtying the filter codes. Besides, we still need to define the global QTAILQ_HEAD(,)COLOBufferFilters in this scenario. Thanks, Hailiang >> >>>> . >>>> >>> >> >> >> > > > . >
diff --git a/migration/colo.c b/migration/colo.c index 235578b..86a7638 100644 --- a/migration/colo.c +++ b/migration/colo.c @@ -104,10 +104,26 @@ static void secondary_vm_do_failover(void) } } +static void colo_set_filter_status(NetFilterState *nf, void *opaque, + Error **errp) +{ + char colo_filter[128]; + char *name = object_get_canonical_path_component(OBJECT(nf)); + char *status = opaque; + + snprintf(colo_filter, sizeof(colo_filter), "%scolo", nf->netdev_id); + if (strcmp(colo_filter, name)) { + return; + } + object_property_set_str(OBJECT(nf), status, "status", errp); +} + static void primary_vm_do_failover(void) { MigrationState *s = migrate_get_current(); int old_state; + char *status; + Error *local_err = NULL; migrate_set_state(&s->state, MIGRATION_STATUS_COLO, MIGRATION_STATUS_COMPLETED); @@ -131,6 +147,14 @@ static void primary_vm_do_failover(void) old_state); return; } + + status = g_strdup("disable"); + qemu_foreach_netfilter(colo_set_filter_status, status, &local_err); + g_free(status); + if (local_err) { + error_report_err(local_err); + } + /* Notify COLO thread that failover work is finished */ qemu_sem_post(&s->colo_sem); } @@ -404,11 +428,19 @@ static void colo_process_checkpoint(MigrationState *s) { QEMUSizedBuffer *buffer = NULL; int64_t current_time, checkpoint_time = qemu_clock_get_ms(QEMU_CLOCK_HOST); + char *status; Error *local_err = NULL; int ret; failover_init_state(); + status = g_strdup("enable"); + qemu_foreach_netfilter(colo_set_filter_status, status, &local_err); + g_free(status); + if (local_err) { + goto out; + } + s->rp_state.from_dst_file = qemu_file_get_return_path(s->to_dst_file); if (!s->rp_state.from_dst_file) { error_report("Open QEMUFile from_dst_file failed");
Enable all buffer filters that added by COLO while go into COLO process, and disable them while exit COLO. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Cc: Jason Wang <jasowang@redhat.com> Cc: Yang Hongyang <hongyang.yang@easystack.cn> --- v14: - New patch --- migration/colo.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)