Re: [Qemu-devel] [PATCH COLO-Frame v14 37/40] COLO: enable buffer filters for PVM

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

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

>>
>>>> .
>>>>
>>>
>>
>>
>>
>
>
> .
>