Re: [Xen-devel] [PATCH V3 2/7] COLO-Proxy: Setup userspace colo-proxy on primary side

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Feb 21, 2017 at 07:03:29PM +0800, Zhang Chen wrote:
> 
> 
> On 02/21/2017 05:53 PM, Wei Liu wrote:
> > On Tue, Feb 21, 2017 at 10:57:46AM +0800, Zhang Chen wrote:
> > > 
> > > On 02/20/2017 11:55 PM, Wei Liu wrote:
> > > > On Fri, Feb 17, 2017 at 10:18:24AM +0800, Zhang Chen wrote:
> > > > > In this patch we close kernel COLO-Proxy on primary side.
> > > > > 
> > > > > Signed-off-by: Zhang Chen <zhangchen.fnst@xxxxxxxxxxxxxx>
> > > > > ---
> > > > >    tools/libxl/libxl_colo_proxy.c | 27 +++++++++++++++++++++++++++
> > > > >    tools/libxl/libxl_colo_save.c  |  9 +++++++--
> > > > >    2 files changed, 34 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/tools/libxl/libxl_colo_proxy.c 
> > > > > b/tools/libxl/libxl_colo_proxy.c
> > > > > index 0983f42..dd902fc 100644
> > > > > --- a/tools/libxl/libxl_colo_proxy.c
> > > > > +++ b/tools/libxl/libxl_colo_proxy.c
> > > > > @@ -152,6 +152,10 @@ int colo_proxy_setup(libxl__colo_proxy_state 
> > > > > *cps)
> > > > >        STATE_AO_GC(cps->ao);
> > > > > +    /* If enable userspace proxy mode, we don't need setup kernel 
> > > > > proxy */
> > > > > +    if (cps->is_userspace_proxy)
> > > > > +        return 0;
> > > > > +
> > > > >        skfd = socket(PF_NETLINK, SOCK_RAW, NETLINK_COLO);
> > > > >        if (skfd < 0) {
> > > > >            LOGD(ERROR, ao->domid, "can not create a netlink socket: 
> > > > > %s", strerror(errno));
> > > > > @@ -222,6 +226,13 @@ out:
> > > > >    void colo_proxy_teardown(libxl__colo_proxy_state *cps)
> > > > >    {
> > > > > +    /*
> > > > > +     * If enable userspace proxy mode,
> > > > > +     * we don't need teardown kernel proxy
> > > > > +     */
> > > > > +    if (cps->is_userspace_proxy)
> > > > > +        return;
> > > > > +
> > > > >        if (cps->sock_fd >= 0) {
> > > > >            close(cps->sock_fd);
> > > > >            cps->sock_fd = -1;
> > > > > @@ -232,6 +243,13 @@ void colo_proxy_teardown(libxl__colo_proxy_state 
> > > > > *cps)
> > > > >    void colo_proxy_preresume(libxl__colo_proxy_state *cps)
> > > > >    {
> > > > > +    /*
> > > > > +     * If enable userspace proxy mode,
> > > > > +     * we don't need preresume kernel proxy
> > > > > +     */
> > > > > +    if (cps->is_userspace_proxy)
> > > > > +        return;
> > > > > +
> > > > >        colo_proxy_send(cps, NULL, 0, COLO_CHECKPOINT);
> > > > >        /* TODO: need to handle if the call fails... */
> > > > >    }
> > > > > @@ -262,6 +280,15 @@ int 
> > > > > colo_proxy_checkpoint(libxl__colo_proxy_state *cps,
> > > > >        STATE_AO_GC(cps->ao);
> > > > > +    /*
> > > > > +     * enable userspace proxy mode, tmp sleep.
> > > > > +     * then we will add qemu API support this func.
> > > > > +     */
> > > > > +    if (cps->is_userspace_proxy) {
> > > > > +        sleep(timeout_us / 1000000);
> > > > usleep is better.
> > > OK.
> > > 
> > > > But in general I don't think sleeping in libxl is a good idea.
> > > > What is the reason that you need to sleep here?
> > > In here we use this sleep to keep COLO period checkpoint,
> > > We can not do checkpoint continuously, that will make performance poor.
> > > After 7/7 we change this to
> > > ret = colo_userspace_proxy_recv(cps, recvbuff, timeout_us);
> > > 
> > I don't fully understand. Say, if I just use COLO with this sleep, will
> > it work?
> 
> Yes, but COLO just have period checkpoint.
> for example, in this mode, we set 10 sec to do one checkpoint,
> the guest's net packet will be buffered 0-10 sec before send to
> client.because we must ensure primary guest status same with secondary
> guest(need do a checkpoint), When we have colo-compare, if primary guest's
> packet same
> with secondary guest's packet we send packet to client immediately,
> else have different packet we alse send packet to client immediately and
> with a checkpoint notify to colo-frame. That's make colo performance greater
> than period mode.
> 

OK, in short, this is for periodical checkpoint mode. I still think it
is a bad idea to invoke (u)sleep in libxl, but since you're going to
remove it anyway, there is no point in making things more complicated
than necessary.

But I want to suggest you improve the comment a bit.

1. Properly capitalise the sentence.
2. Replace "tmp sleep" with "sleeping temporarily for XXX mode".
3. Clarify the QEMU API part (why is that relevant to the sleep or
   whatever comes next).

I don't like to nit-pick too much about language, but keep in mind that
other people may want to contribute to COLO project so it is important
to have comments as accurate and clear as possible.

Wei.

> 
> Thanks
> Zhang Chen
> 
> > 
> > > Thanks
> > > Zhang Chen
> > > 
> > > > Wei.
> > > > 
> > > > 
> > > > .
> > > > 
> > > -- 
> > > Thanks
> > > Zhang Chen
> > > 
> > > 
> > > 
> > 
> > .
> > 
> 
> -- 
> Thanks
> Zhang Chen
> 
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel