[Spice-commits] 2 commits - server/red_client_shared_cache.h server/red_dispatcher.c server/red_dispatcher.h server/red_worker.c server/spice-qxl.h server/spice-server.syms
Christophe Fergau
teuf at kemper.freedesktop.org
Mon Jun 29 05:16:47 PDT 2015
server/red_client_shared_cache.h | 24 ++++++++++++------------
server/red_dispatcher.c | 10 ++++++++++
server/red_dispatcher.h | 1 +
server/red_worker.c | 35 ++++++++++++++++++++++-------------
server/spice-qxl.h | 3 +++
server/spice-server.syms | 5 +++++
6 files changed, 53 insertions(+), 25 deletions(-)
New commits:
commit e4a42e50c2d0558583301309feb6f349ca4af0c5
Author: Sandy Stutsman <sstutsma at redhat.com>
Date: Fri Jun 26 11:59:13 2015 -0400
Lock the pixmap image cache for the entire fill_bits call
Locking the individual calls that access the pixmap cache in fill_bits is
not adequately thread safe. Often a windows guest with multiple monitors
will be sending the same image via different threads. Both threads can
be in fill_bits at the same time making changes to the cache for the same
image. This can result in images being deleted before all the client
channels are finished with them or with the same image being send multiple
times. Here's what can happen with out the lock in fill_bits
On the server in red_worker.c:fill_bits
Thread 1 calls pixmap_cache_hit for Image A and finds it isn't in cache
Thread 2 calls pixmap_cache_hit for Image A and finds it isn't in cache
Thread 1 adds Image 1 to pixmap_cache (1x)
Thread 2 adds Image 1 to pixmap_cache (2x)
On the client
Channel 1 adds Image A to image_cache (1x)
Channel 2 replaces Image A in image_cache (1x)
On server
Thread 1 sends Image A rendering commands
Thread N removes Image A from pixmap_cache (image remains - 1x)
Thread 2 sends Image A rendering commands
On client
Channe1 renders from Image A
Channel N removes Image a from image_cache (image is completely removed)
Channel2 render command hangs waiting for Image A
diff --git a/server/red_client_shared_cache.h b/server/red_client_shared_cache.h
index 821ee18..7feb28e 100644
--- a/server/red_client_shared_cache.h
+++ b/server/red_client_shared_cache.h
@@ -36,13 +36,12 @@
#define CHANNEL_FROM_RCC(rcc) SPICE_CONTAINEROF((rcc)->channel, CHANNEL, common.base);
-static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc)
+static int FUNC_NAME(unlocked_hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc)
{
NewCacheItem *item;
uint64_t serial;
serial = red_channel_client_get_message_serial(&dcc->common.base);
- pthread_mutex_lock(&cache->lock);
item = cache->hash_table[CACHE_HASH_KEY(id)];
while (item) {
@@ -57,15 +56,22 @@ static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelC
}
item = item->next;
}
- pthread_mutex_unlock(&cache->lock);
return !!item;
}
-static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t id, int lossy)
+static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc)
{
- NewCacheItem *item;
+ int hit;
pthread_mutex_lock(&cache->lock);
+ hit = FUNC_NAME(unlocked_hit)(cache,id,lossy, dcc);
+ pthread_mutex_unlock(&cache->lock);
+ return hit;
+}
+
+static int FUNC_NAME(unlocked_set_lossy)(CACHE *cache, uint64_t id, int lossy)
+{
+ NewCacheItem *item;
item = cache->hash_table[CACHE_HASH_KEY(id)];
@@ -76,11 +82,10 @@ static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t id, int lossy)
}
item = item->next;
}
- pthread_mutex_unlock(&cache->lock);
return !!item;
}
-static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, DisplayChannelClient *dcc)
+static int FUNC_NAME(unlocked_add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, DisplayChannelClient *dcc)
{
NewCacheItem *item;
uint64_t serial;
@@ -91,15 +96,12 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, D
item = spice_new(NewCacheItem, 1);
serial = red_channel_client_get_message_serial(&dcc->common.base);
- pthread_mutex_lock(&cache->lock);
-
if (cache->generation != dcc->CACH_GENERATION) {
if (!dcc->pending_pixmaps_sync) {
red_channel_client_pipe_add_type(
&dcc->common.base, PIPE_ITEM_TYPE_PIXMAP_SYNC);
dcc->pending_pixmaps_sync = TRUE;
}
- pthread_mutex_unlock(&cache->lock);
free(item);
return FALSE;
}
@@ -112,7 +114,6 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, D
if (!(tail = (NewCacheItem *)ring_get_tail(&cache->lru)) ||
tail->sync[dcc->common.id] == serial) {
cache->available += size;
- pthread_mutex_unlock(&cache->lock);
free(item);
return FALSE;
}
@@ -144,7 +145,6 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, D
memset(item->sync, 0, sizeof(item->sync));
item->sync[dcc->common.id] = serial;
cache->sync[dcc->common.id] = serial;
- pthread_mutex_unlock(&cache->lock);
return TRUE;
}
diff --git a/server/red_worker.c b/server/red_worker.c
index f3d8ad9..0ad16e2 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -6686,9 +6686,9 @@ static inline void red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
if ((image->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME)) {
spice_assert(image->descriptor.width * image->descriptor.height > 0);
if (!(io_image->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME)) {
- if (pixmap_cache_add(dcc->pixmap_cache, image->descriptor.id,
- image->descriptor.width * image->descriptor.height, is_lossy,
- dcc)) {
+ if (pixmap_cache_unlocked_add(dcc->pixmap_cache, image->descriptor.id,
+ image->descriptor.width * image->descriptor.height, is_lossy,
+ dcc)) {
io_image->descriptor.flags |= SPICE_IMAGE_FLAGS_CACHE_ME;
dcc->send_data.pixmap_cache_items[dcc->send_data.num_pixmap_cache_items++] =
image->descriptor.id;
@@ -6733,11 +6733,12 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
if (simage->descriptor.flags & SPICE_IMAGE_FLAGS_HIGH_BITS_SET) {
image.descriptor.flags = SPICE_IMAGE_FLAGS_HIGH_BITS_SET;
}
+ pthread_mutex_lock(&dcc->pixmap_cache->lock);
if ((simage->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME)) {
int lossy_cache_item;
- if (pixmap_cache_hit(dcc->pixmap_cache, image.descriptor.id,
- &lossy_cache_item, dcc)) {
+ if (pixmap_cache_unlocked_hit(dcc->pixmap_cache, image.descriptor.id,
+ &lossy_cache_item, dcc)) {
dcc->send_data.pixmap_cache_items[dcc->send_data.num_pixmap_cache_items++] =
image.descriptor.id;
if (can_lossy || !lossy_cache_item) {
@@ -6754,10 +6755,11 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
spice_assert(bitmap_palette_out == NULL);
spice_assert(lzplt_palette_out == NULL);
stat_inc_counter(display_channel->cache_hits_counter, 1);
+ pthread_mutex_unlock(&dcc->pixmap_cache->lock);
return FILL_BITS_TYPE_CACHE;
} else {
- pixmap_cache_set_lossy(dcc->pixmap_cache, simage->descriptor.id,
- FALSE);
+ pixmap_cache_unlocked_set_lossy(dcc->pixmap_cache, simage->descriptor.id,
+ FALSE);
image.descriptor.flags |= SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME;
}
}
@@ -6771,6 +6773,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
surface_id = simage->u.surface.surface_id;
if (!validate_surface(worker, surface_id)) {
rendering_incorrect("SPICE_IMAGE_TYPE_SURFACE");
+ pthread_mutex_unlock(&dcc->pixmap_cache->lock);
return FILL_BITS_TYPE_SURFACE;
}
@@ -6785,6 +6788,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
&bitmap_palette_out, &lzplt_palette_out);
spice_assert(bitmap_palette_out == NULL);
spice_assert(lzplt_palette_out == NULL);
+ pthread_mutex_unlock(&dcc->pixmap_cache->lock);
return FILL_BITS_TYPE_SURFACE;
}
case SPICE_IMAGE_TYPE_BITMAP: {
@@ -6816,6 +6820,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
}
spice_marshaller_add_ref_chunks(m, bitmap->data);
+ pthread_mutex_unlock(&dcc->pixmap_cache->lock);
return FILL_BITS_TYPE_BITMAP;
} else {
red_display_add_image_to_pixmap_cache(rcc, simage, &image,
@@ -6833,6 +6838,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
}
spice_assert(!comp_send_data.is_lossy || can_lossy);
+ pthread_mutex_unlock(&dcc->pixmap_cache->lock);
return (comp_send_data.is_lossy ? FILL_BITS_TYPE_COMPRESS_LOSSY :
FILL_BITS_TYPE_COMPRESS_LOSSLESS);
}
@@ -6846,11 +6852,12 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
spice_assert(bitmap_palette_out == NULL);
spice_assert(lzplt_palette_out == NULL);
spice_marshaller_add_ref_chunks(m, image.u.quic.data);
+ pthread_mutex_unlock(&dcc->pixmap_cache->lock);
return FILL_BITS_TYPE_COMPRESS_LOSSLESS;
default:
spice_error("invalid image type %u", image.descriptor.type);
}
-
+ pthread_mutex_unlock(&dcc->pixmap_cache->lock);
return 0;
}
commit 6d4e58f70ddf9c178e2cc20dd5a3ade8b95fd142
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Fri Jun 19 11:56:05 2015 +0100
server: allows to set maximum monitors
spice-server will attempt to limit number of monitors.
Guest machine can send monitor list it accepts. Limiting the number sent
by guest will limit the number of monitors client will try to enable.
The guest usually see client monitors enabled and start using it so
not seeing client monitor won't try to enable more monitor.
In this case the additional monitor guest can support will always be
seen as heads with no attached monitors.
This allows limiting monitors number without changing guest drivers.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
index 74adaa2..a2ef226 100644
--- a/server/red_dispatcher.c
+++ b/server/red_dispatcher.c
@@ -66,6 +66,7 @@ struct RedDispatcher {
Ring async_commands;
pthread_mutex_t async_lock;
QXLDevSurfaceCreate surface_create;
+ unsigned int max_monitors;
};
extern uint32_t streaming_video;
@@ -693,6 +694,7 @@ static void red_dispatcher_monitors_config_async(RedDispatcher *dispatcher,
payload.base.cmd = async_command_alloc(dispatcher, message, cookie);
payload.monitors_config = monitors_config;
payload.group_id = group_id;
+ payload.max_monitors = dispatcher->max_monitors;
dispatcher_send_message(&dispatcher->dispatcher, message, &payload);
}
@@ -987,6 +989,12 @@ void spice_qxl_monitors_config_async(QXLInstance *instance, QXLPHYSICAL monitors
}
SPICE_GNUC_VISIBLE
+void spice_qxl_set_max_monitors(QXLInstance *instance, unsigned int max_monitors)
+{
+ instance->st->dispatcher->max_monitors = MAX(1u, max_monitors);
+}
+
+SPICE_GNUC_VISIBLE
void spice_qxl_driver_unload(QXLInstance *instance)
{
red_dispatcher_driver_unload(instance->st->dispatcher);
@@ -1110,6 +1118,8 @@ void red_dispatcher_init(QXLInstance *qxl)
red_dispatcher->base.destroy_surface_wait = qxl_worker_destroy_surface_wait;
red_dispatcher->base.loadvm_commands = qxl_worker_loadvm_commands;
+ red_dispatcher->max_monitors = UINT_MAX;
+
qxl->st->qif->get_init_info(qxl, &init_info);
init_data.memslot_id_bits = init_info.memslot_id_bits;
diff --git a/server/red_dispatcher.h b/server/red_dispatcher.h
index 907b7c7..70b8a62 100644
--- a/server/red_dispatcher.h
+++ b/server/red_dispatcher.h
@@ -200,6 +200,7 @@ typedef struct RedWorkerMessageMonitorsConfigAsync {
RedWorkerMessageAsync base;
QXLPHYSICAL monitors_config;
int group_id;
+ unsigned int max_monitors;
} RedWorkerMessageMonitorsConfigAsync;
typedef struct RedWorkerMessageDriverUnload {
diff --git a/server/red_worker.c b/server/red_worker.c
index 515262d..f3d8ad9 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -11217,11 +11217,13 @@ static inline void red_monitors_config_item_add(DisplayChannelClient *dcc)
}
static void worker_update_monitors_config(RedWorker *worker,
- QXLMonitorsConfig *dev_monitors_config)
+ QXLMonitorsConfig *dev_monitors_config,
+ unsigned int max_monitors)
{
int heads_size;
MonitorsConfig *monitors_config;
int i;
+ unsigned int count = MIN(dev_monitors_config->count, max_monitors);
monitors_config_decref(worker->monitors_config);
@@ -11235,13 +11237,13 @@ static void worker_update_monitors_config(RedWorker *worker,
dev_monitors_config->heads[i].width,
dev_monitors_config->heads[i].height);
}
- heads_size = dev_monitors_config->count * sizeof(QXLHead);
+ heads_size = count * sizeof(QXLHead);
worker->monitors_config = monitors_config =
spice_malloc(sizeof(*monitors_config) + heads_size);
monitors_config->refs = 1;
monitors_config->worker = worker;
- monitors_config->count = dev_monitors_config->count;
- monitors_config->max_allowed = dev_monitors_config->max_allowed;
+ monitors_config->count = count;
+ monitors_config->max_allowed = MIN(dev_monitors_config->max_allowed, max_monitors);
memcpy(monitors_config->heads, dev_monitors_config->heads, heads_size);
}
@@ -11651,7 +11653,7 @@ static void handle_dev_monitors_config_async(void *opaque, void *payload)
dev_monitors_config->max_allowed);
return;
}
- worker_update_monitors_config(worker, dev_monitors_config);
+ worker_update_monitors_config(worker, dev_monitors_config, msg->max_monitors);
red_worker_push_monitors_config(worker);
}
diff --git a/server/spice-qxl.h b/server/spice-qxl.h
index 31ff742..dd49a86 100644
--- a/server/spice-qxl.h
+++ b/server/spice-qxl.h
@@ -97,6 +97,9 @@ void spice_qxl_monitors_config_async(QXLInstance *instance, QXLPHYSICAL monitors
int group_id, uint64_t cookie);
/* since spice 0.12.3 */
void spice_qxl_driver_unload(QXLInstance *instance);
+/* since spice 0.12.6 */
+void spice_qxl_set_max_monitors(QXLInstance *instance,
+ unsigned int max_monitors);
typedef struct QXLDrawArea {
uint8_t *buf;
diff --git a/server/spice-server.syms b/server/spice-server.syms
index 2193811..3c7d945 100644
--- a/server/spice-server.syms
+++ b/server/spice-server.syms
@@ -153,3 +153,8 @@ global:
spice_server_get_best_record_rate;
spice_server_set_record_rate;
} SPICE_SERVER_0.12.4;
+
+SPICE_SERVER_0.12.6 {
+global:
+ spice_qxl_set_max_monitors;
+} SPICE_SERVER_0.12.5;
More information about the Spice-commits
mailing list