Re: [PATCH 10/14 v4] xen/arm: vpl011: Modify xenconsole to support multiple consoles

From: Stefano Stabellini <sstabellini@kernel.org>
To: Bhupinder Thakur <bhupinder.thakur@linaro.org>
Cc: xen-devel@lists.xenproject.org,
	Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Wei Liu <wei.liu2@citrix.com>
Subject: Re: [PATCH 10/14 v4] xen/arm: vpl011: Modify xenconsole to support multiple consoles
Date: Wed, 7 Jun 2017 10:54:05 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.10.1706071053070.26108@sstabellini-ThinkPad-X260> (raw)
In-Reply-To: <CACtJ1JSUb4zJJ=_pzBm454cA71RG5DFX+FZaqn2FpaZ3WTQtuQ@mail.gmail.com>

On Wed, 7 Jun 2017, Bhupinder Thakur wrote:
> Hi Stefano,
> 
> Thanks for your comments.
> 
> >> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> >> index c5dd08d..db73e10 100644
> >> --- a/tools/console/daemon/io.c
> >> +++ b/tools/console/daemon/io.c
> >> @@ -90,12 +90,15 @@ struct buffer {
> >>  };
> >>
> >>  struct console {
> >> +     char *xsname;
> >
> > How is xsname useful? It doesn't look like it is used anywhere except
> > init, right?
> Yes. I will remove it from the console structure.
> 
> >
> >
> >> +     char *ttyname;
> >>       int master_fd;
> >>       int master_pollfd_idx;
> >>       int slave_fd;
> >>       int log_fd;
> >>       struct buffer buffer;
> >> -     char *conspath;
> >> +     char *xspath;
> >
> > A simple trick to make patch easier to handle is to separate out changes
> > like this one: renaming conspath to xspath causes a lot of churn, which
> > ends up all mixed up with other meaningful changes.
> >
> I thought that xspath is a more appropriate name than conspath because
> it tells that it is related to
> xenstore. I could keep it unchanged. Let me know. Else I will
> introduce this in a separate patch.

Yes, rename it but in a separate patch

> >
> >> +     char *log_suffix;
> >>       int ring_ref;
> >>       xenevtchn_port_or_error_t local_port;
> >>       xenevtchn_port_or_error_t remote_port;
> >> @@ -103,6 +106,23 @@ struct console {
> >>       struct domain *d;
> >>  };
> >>
> >> +struct console_data {
> >> +     char *xsname;
> >> +     char *ttyname;
> >> +     char *log_suffix;
> >> +};
> >> +
> >> +static struct console_data console_data[] = {
> >> +
> >> +     {
> >> +             .xsname = "/console",
> >> +             .ttyname = "tty",
> >> +             .log_suffix = "",
> >> +     },
> >> +};
> >> +
> >> +#define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
> >> +
> >>  struct domain {
> >>       int domid;
> >>       bool is_dead;
> >> @@ -112,11 +132,90 @@ struct domain {
> >>       int xce_pollfd_idx;
> >>       int event_count;
> >>       long long next_period;
> >> -     struct console console;
> >> +     struct console console[MAX_CONSOLE];
> >>  };
> >>
> >> +static void buffer_append(struct console *con, unsigned int data)
> >>  {
> >>       struct buffer *buffer = &con->buffer;
> >> +     struct xencons_interface *intf = con->interface;
> >> +     xenevtchn_port_or_error_t rxport = (xenevtchn_port_or_error_t)data;
> >>       struct domain *dom = con->d;
> >>       XENCONS_RING_IDX cons, prod, size;
> >> -     struct xencons_interface *intf = con->interface;
> >> +
> >> +     /* If incoming data is not for the current console then ignore. */
> >> +     if (con->local_port != rxport)
> >> +             return;
> >>
> >>       cons = intf->out_cons;
> >>       prod = intf->out_prod;
> >> @@ -427,6 +541,9 @@ static int console_create_tty(struct console *con)
> >>       struct termios term;
> >>       struct domain *dom = con->d;
> >>
> >> +     if (!console_enabled(con))
> >> +             return 1;
> >
> > Is this actually useful? It doesn't look like the rest code changed in
> > regards to console_create_tty.
> I think this check in not required as it would be called only if the
> console was initialized.
> I will remove the check.

OK

> >
> >
> >
> >>       assert(con->slave_fd == -1);
> >>       assert(con->master_fd == -1);
> >>
> >> @@ -594,15 +711,16 @@ static int console_create_ring(struct console *con)
> >>
> >>       con->local_port = -1;
> >>       con->remote_port = -1;
> >> -     if (dom->xce_handle != NULL)
> >> -             xenevtchn_close(dom->xce_handle);
> >>
> >> -     /* Opening evtchn independently for each console is a bit
> >> -      * wasteful, but that's how the code is structured... */
> >> -     dom->xce_handle = xenevtchn_open(NULL, 0);
> >> -     if (dom->xce_handle == NULL) {
> >> -             err = errno;
> >> -             goto out;
> >> +     if (dom->xce_handle == NULL)
> >> +     {
> >> +             /* Opening evtchn independently for each console is a bit
> >> +              * wasteful, but that's how the code is structured... */
> >> +             dom->xce_handle = xenevtchn_open(NULL, 0);
> >> +             if (dom->xce_handle == NULL) {
> >> +                     err = errno;
> >> +                     goto out;
> >> +             }
> >
> > I think we need to do this per console actually, see below
> >
> >
> >>       }
> >>
> >>       rc = xenevtchn_bind_interdomain(dom->xce_handle,
> >> @@ -1092,14 +1282,13 @@ void handle_io(void)
> >>                       if ((now+5) > d->next_period) {
> >>                               d->next_period = now + RATE_LIMIT_PERIOD;
> >>                               if (d->event_count >= RATE_LIMIT_ALLOWANCE) {
> >> -                                     (void)xenevtchn_unmask(d->xce_handle, con->local_port);
> >> +                                     console_iter_void_arg1(d, console_event_unmask);
> >>                               }
> >>                               d->event_count = 0;
> >>                       }
> >>               }
> >> @@ -1107,28 +1296,15 @@ void handle_io(void)
> >>                                   d->next_period < next_timeout)
> >>                                       next_timeout = d->next_period;
> >>                       } else if (d->xce_handle != NULL) {
> >> -                             if (discard_overflowed_data ||
> >> -                                 !con->buffer.max_capacity ||
> >> -                                 con->buffer.size < con->buffer.max_capacity) {
> >> -                                     int evtchn_fd = xenevtchn_fd(d->xce_handle);
> >> -                                     d->xce_pollfd_idx = set_fds(evtchn_fd,
> >> -                                                                 POLLIN|POLLPRI);
> >> +                                     if (console_iter_bool_arg1(d, buffer_available))
> >> +                                     {
> >> +                                             int evtchn_fd = xenevtchn_fd(d->xce_handle);
> >> +                                             d->xce_pollfd_idx = set_fds(evtchn_fd,
> >> +                                                                                                     POLLIN|POLLPRI);
> >> +                                     }
> >>                               }
> >
> > Is there a reason why we have one xce_pollfd_idx, xce_handle,
> > next_period and event_count per domain, rather than per console?
> >
> > It is strange to set xce_pollfd_idx if at least one console of the
> > domain has enough buffer availability. Similarly, it is strange to count
> > the next_period on a per domain basis, regardless of the number of
> > consoles. It would be natural to do it at the console level.
> 
> I tried to reuse the same event channel for handling multiple consoles
> since an event channel can handle multiple connections by allocating
> unique local ports. Considering that there will not be many consoles
> active at the same time, I thought it might be ok to reuse the same
> event channel.
> 
> I agree that it is natural to make this per console. Let me know if I
> should make it per console.

Yes, please.

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