Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization

[Jason Wang]

On 05/06/2016 01:42 PM, Zhang Chen wrote:
>
>
> On 04/29/2016 10:03 AM, Jason Wang wrote:
>>
>> On 04/28/2016 05:04 PM, Zhang Chen wrote:
>>>
>>> On 04/28/2016 04:17 PM, Jason Wang wrote:
>>>> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>>>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>>>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>>>>> Error **errp)
>>>>>>>> +{
>>>>>>>> +    CompareState *s = COLO_COMPARE(obj);
>>>>>>>> +
>>>>>>>> +    g_free(s->outdev);
>>>>>>>> +    s->outdev = g_strdup(value);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>> + * called from the main thread on the primary
>>>>>>>> + * to setup colo-compare.
>>>>>>>> + */
>>>>>>>> +static void colo_compare_complete(UserCreatable *uc, Error
>>>>>>>> **errp)
>>>>>>>> +{
>>>>>>>> +    CompareState *s = COLO_COMPARE(uc);
>>>>>>>> +
>>>>>>>> +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>>>>> +        error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>>>>> +                   "'secondary_in','outdev' property set");
>>>>>>>> +        return;
>>>>>>>> +    } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>>>>> +               !strcmp(s->sec_indev, s->outdev) ||
>>>>>>>> +               !strcmp(s->pri_indev, s->sec_indev)) {
>>>>>>>> +        error_setg(errp, "'indev' and 'outdev' could not be
>>>>>>>> same "
>>>>>>>> +                   "for compare module");
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>>>>> +    if (s->chr_pri_in == NULL) {
>>>>>>>> +        error_setg(errp, "Primary IN Device '%s' not found",
>>>>>>>> +                   s->pri_indev);
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>>>> +    if (s->chr_sec_in == NULL) {
>>>>>>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>>>> +                   s->sec_indev);
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>>>>>>> +    if (s->chr_out == NULL) {
>>>>>>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>>>> +                          compare_pri_chr_in, NULL, s);
>>>>>>>> +
>>>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>>>> +                          compare_sec_chr_in, NULL, s);
>>>>>>>> +
>>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>>> would
>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>> extra
>>>>>> synchronizations?
>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>>> thread. Through this way, there's no need for extra synchronization
>>>> with
>>>> main loop.
>>> I get your point, but how to do this.
>>> Now, we use qemu_chr_add_handlers to do this job.
>> You probably want to start a new main loop in colo comparing thread.
>
>
> IIUC, do you mean
> - remove char device read_handler
>
>  ↓at colo comparing thread↓
> while (true) {
> - blocking read packet from char device with select(2)/poll(2)...
> - compare packet
> }

Yes, something like this.

>
> This solution will lead comparing packet and reading packet in serial.
> But i don't know if this will have a good performance.

This probably won't have the best performance but it simplify lots of
things. Actually doing it in main loop will slow down all other I/O
processing. Consider colo can only handling userspace network traffic
now, we could start from this. For performance, it needs lots of other
stuff: I think the most important thing is to add vhost support.

Thanks

>
>>>
>>> Thanks
>>> zhangchen
>>
>>
>> .
>>
>