>
> Hi,
>
> On Mon, Jun 17, 2019 at 04:40:08PM +0100, Frediano Ziglio wrote:
> > As we don't use any token there's no reason to not queue directly instead
> > of passing through RedCharDevice.
> > This will make easier to limit the queue which currently is unlimited.
> >
> > RedCharDevice flow control has some problems:
> > - it's really designed with VDI in mind. This for SpiceVMC would cause
> > code implementation to be confusing having to satisfy the agent token
> > handling;
> > - it's using items as unit not allowing counting bytes;
> > - it duplicates some queue management between RedChannelClient;
> > - it's broken (see comments in char-device.h);
> > - it forces "clients" to behave in some way not altering for instance the
> > queued items (which is very useful if items can be collapsed together);
> > - it replicates the token handling on the device queue too. This could
> > seems right but is not that if you have a network card going @ 1 GBit/s
> > you are able to upload more than 1 Gbit/s just using multiple
> > connections. The kernel will use a single queue for the network interface
> > limiting and sharing de facto the various buffers between the multiple
> > connections.
> >
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> > Changes in v2:
> > - more commit message comments
> > ---
> > server/spicevmc.c | 15 +++++----------
> > 1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > index 84bbb73c2..8c41573ae 100644
> > --- a/server/spicevmc.c
> > +++ b/server/spicevmc.c
> > @@ -360,29 +360,24 @@ static RedPipeItem
> > *spicevmc_chardev_read_msg_from_dev(RedCharDevice *self,
> >
> > msg_item_compressed = try_compress_lz4(channel, n, msg_item);
> > if (msg_item_compressed != NULL) {
> > - return &msg_item_compressed->base;
> > + red_channel_client_pipe_add_push(channel->rcc,
> > &msg_item_compressed->base);
> > + return NULL;
> > }
> > #endif
> > stat_inc_counter(channel->out_data, n);
> > msg_item->uncompressed_data_size = n;
> > msg_item->buf_used = n;
> > - return &msg_item->base;
> > - } else {
> > - channel->pipe_item = msg_item;
> > + red_channel_client_pipe_add_push(channel->rcc, &msg_item->base);
> > return NULL;
> > }
> > + channel->pipe_item = msg_item;
> > + return NULL;
> > }
> >
> > static void spicevmc_chardev_send_msg_to_client(RedCharDevice *self,
> > RedPipeItem *msg,
> > RedClient *client)
> > {
> > - RedCharDeviceSpiceVmc *vmc = RED_CHAR_DEVICE_SPICEVMC(self);
> > - RedVmcChannel *channel = RED_VMC_CHANNEL(vmc->channel);
> > -
> > - spice_assert(red_channel_client_get_client(channel->rcc) == client);
> > - red_pipe_item_ref(msg);
> > - red_channel_client_pipe_add_push(channel->rcc, msg);
> > }
>
> Is it worth to 1) update char-device.h that this is is not used
It does not seem a great idea to me.
I would just add to send_msg_to_client that this callback can be
NULL if we allow that. And probably it's a good idea as messages
are accounted for token computation and token callbacks are protected
by a "if (callback != NULL)" check.
Isn't better the change in a follow up patch?
> in spicevmc; 2) update char-device.c with
>
> - klass->send_msg_to_client(dev, msg, client);
> + if (klass->send_msg_to_client) {
> + klass->send_msg_to_client(dev, msg, client);
> + }
>
> and remove this function?;
>
> Patch looks good.
>
> > static void red_port_init_item_free(struct RedPipeItem *base)
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel