Re: [PATCH v6 6/8] chardev/char-mux: implement backend chardev multiplexing

[Marc-André Lureau]

Hi

On Thu, Jan 2, 2025 at 2:22 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
>
> Hi,
>
> First of all Happy New Year :)
>
> On Mon, Dec 30, 2024 at 12:41 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>
> [cut]
>
> > > +
> > > +    for (i = 0; i < d->be_cnt; i++) {
> > > +        written = d->be_written[i] - d->be_min_written;
> > > +        if (written) {
> > > +            /* Written in the previous call so take into account */
> > > +            ret = MIN(written, ret);
> > > +            continue;
> > > +        }
> > > +        r = qemu_chr_fe_write(&d->backends[i], buf, len);
> > > +        if (r < 0 && errno == EAGAIN) {
> > > +            /*
> > > +             * Fail immediately if write would block. Expect to be called
> > > +             * soon on watch wake up.
> > > +             */
> > > +            d->be_eagain_ind = i;
> > > +            return r;
> >
> > But next attempt to write will loop over the same backend again, which
> > will see the "same" write multiple times.
>
> This case is handled by checking the difference between counters
> `d->be_written[i]` and `d->be_min_written`. The idea is that device, which
> already "swallowed" some portion of data, will be skipped from writing to it,
> until it catches up with the stream.

ok, I see. This looks fragile though, I/one will need to do a more
thorough review.

>
> Please take a look into the `char_mux_be_test()` test case, where the
> EAGAIN scenario is tested. The line test-char.c:716 explicitly shows the
> repeat of the write procedure after EAGAIN was received.
>
> >
> > > +        } else if (r < 0) {
> > > +            /*
> > > +             * Ignore all other errors and pretend the entire buffer is
> > > +             * written to avoid this chardev being watched. This device
> > > +             * becomes disabled until the following write succeeds, but
> > > +             * writing continues to others.
> > > +             */
> > > +            r = len;
> > > +        }
> > > +        d->be_written[i] += r;
> > > +        ret = MIN(r, ret);
> > > +    }
> > > +    d->be_min_written += ret;
> > > +
> > > +    return ret;
> > > +}
> >
> > I am not sure what is the correct way to handle write here. This
> > mux-be behaviour is different from mux-fe, since it handles all
> > backend I/Os, and does not select one... it's more of a "mixer",
> > right, Is this wanted?
>
> Right. The intention is to have both consoles simultaneously
> working, for example having char-based tio (over a socket chardev)
> and image-based vnc (over a vc chardev):
>
>     -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
>     -chardev vc,id=vc0 \
>
> and both are connected to the same frontend device.
>
> I agree with you on the "mixer" naming concern, this did not come to
> my mind. As far as I understand the logic of `mux-fe`, it just doesn't seem
> possible to have both frontends running at the same time, because they
> will both generate output, at least that's the case for virtual consoles:
> imagine you have two virtual console frontends working at the same time
> and one backend. Any command you enter from a backend causes the two
> separate frontends to output completely different data.
>
> On the other hand, several backend devices can easily be simultaneously
> attached to one frontend, the analogy is simple: several monitors, several
> keyboards, etc work perfectly fine with a single PC. At least this is how
> I see this, please correct me if I'm wrong.

Whether we talk about multiplexing front-end or back-end, the issues
are similar. In general, mixing input will create issues. Teeing
output is less problematic, except to handle the buffering...

>
> Do you think we need to artificially introduce multiplexing logic to be fully
> compliant with multiplexer naming? It's not hard to do, repeating
> `mux_proc_byte()` from `mux-fe`. In my use-case, I'll still need to disable
> multiplexing in favor of 'mixing', for example with the 'mixer=on' option,
> i.e. '-chardev mux-be,mixer=on,...`. Or do you think it should be some
> completely different beast, something like mixer chardev?

I think it would be saner to have the muxer be selectors: only work
with one selected be or fe. Otherwise, we can run into various issues.

I hope some qemu maintainers can comment too.