Hey,
On Tue, Oct 18, 2016 at 10:28:25AM +0100, Frediano Ziglio wrote:
> The code was quite complicated.
I tried to look at this one, and it indeed is quite complicated... So
far, too much entangled stuff that I feel confident I'm going to make a
meaningful review...
> - a GlzEncDictImageContext was bound to a GlzDrawableInstanceItem
> (1-1 relation);
> - multiple GlzDrawableInstanceItem were attached to RedGlzDrawable;
> - multiple RedGlzDrawable (one for RedClient) were attached to Drawable
> using GlzImageRetention;
> - OOM code require all Glz dictionary used by DisplayChannel to be locked
> at the same time;
> - drawable count computation during OOM was not corrent in case of
> multiple clients;
> - not clear why to call glz_retention_free_drawables or
> glz_retention_detach_drawables.
>
> Now:
> - a GlzEncDictImageContext is bound to a GlzDictItem (still 1-1 relation);
> - multiple GlzDictItem are attached to a Drawable using GlzImageRetention;
> - there is only a glz_retention_free to be called when the structure
> must be destroyed.
By reading/parsing that, executive summary is that
GlzDrawableInstanceItem/RedGlzDrawable have been merged and everything
has been renamed.
>
> Implementation detail:
> - red_drawable_unref returns a gboolean to indicate if the object
> was really freed;
Ok, does it have to be done in this patch rather than before?
> - glz_drawable_count was removed as there are no more RedGlzDrawable;
And this can't be a follow-up?
> - image_encoders_glz_encode_lock and image_encoders_glz_encode_unlock
> were removed and now the locking is handled entirely by ImageEncoders;
Ok, might be doable after that merging/renaming?
> - instead of marking GlzDictItem/GlzDrawableInstanceItem changing
> has_drawable flag contexts are moved to a glz_orphans ring;
And this change could not be done in a preparatory patch?
> - code is smaller (100 lines less in image-encoders.c) and I hope easier
> to read.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/display-channel.c | 57 +++++----
> server/image-encoders.c | 300 +++++++++++++++--------------------------------
> server/image-encoders.h | 27 +++--
> server/red-worker.c | 5 +-
> server/red-worker.h | 2 +-
> 5 files changed, 145 insertions(+), 246 deletions(-)
>
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 69edd35..9a56080 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1185,7 +1185,7 @@ void display_channel_free_glz_drawables(DisplayChannel *display)
> }
> }
>
> -static bool free_one_drawable(DisplayChannel *display, int force_glz_free)
> +static bool free_one_drawable(DisplayChannel *display)
> {
> RingItem *ring_item = ring_get_tail(&display->priv->current_list);
> Drawable *drawable;
> @@ -1196,9 +1196,6 @@ static bool free_one_drawable(DisplayChannel *display, int force_glz_free)
> }
>
> drawable = SPICE_CONTAINEROF(ring_item, Drawable, list_link);
> - if (force_glz_free) {
> - glz_retention_free_drawables(&drawable->glz_retention);
> - }
> drawable_draw(display, drawable);
> container = drawable->tree_item.base.container;
>
> @@ -1210,37 +1207,47 @@ static bool free_one_drawable(DisplayChannel *display, int force_glz_free)
> void display_channel_current_flush(DisplayChannel *display, int surface_id)
> {
> while (!ring_is_empty(&display->priv->surfaces[surface_id].current_list)) {
> - free_one_drawable(display, FALSE);
> + free_one_drawable(display);
> }
> current_remove_all(display, surface_id);
> }
>
> void display_channel_free_some(DisplayChannel *display)
> {
> - int n = 0;
> + int left_to_free = RED_RELEASE_BUNCH_SIZE;
> DisplayChannelClient *dcc;
> GListIter iter;
>
> - spice_debug("#draw=%d, #glz_draw=%d", display->priv->drawable_count,
> - display->priv->encoder_shared_data.glz_drawable_count);
> + /* As we are trying to release some memory as requested by guest driver
> + * loop througth Glz freeing drawables which have no corresponding Drawable
> + * as this will cause guest memory to be freed. */
> + spice_debug("#draw=%d", display->priv->drawable_count);
> FOREACH_DCC(display, iter, dcc) {
> ImageEncoders *encoders = dcc_get_encoders(dcc);
>
> - // encoding using the dictionary is prevented since the following operations might
> - // change the dictionary
> - if (image_encoders_glz_encode_lock(encoders)) {
> - n = image_encoders_free_some_independent_glz_drawables(encoders);
> - }
> + left_to_free -= image_encoders_free_some_independent_glz_drawables(encoders, left_to_free);
> }
>
> - while (!ring_is_empty(&display->priv->current_list) && n++ < RED_RELEASE_BUNCH_SIZE) {
> - free_one_drawable(display, TRUE);
> - }
> + if (left_to_free > 0) {
> + int saved_to_free = left_to_free;
>
> - FOREACH_DCC(display, iter, dcc) {
> - ImageEncoders *encoders = dcc_get_encoders(dcc);
> + while (left_to_free > 0 && !ring_is_empty(&display->priv->current_list)) {
> + free_one_drawable(display);
> + --left_to_free;
> + }
>
> - image_encoders_glz_encode_unlock(encoders);
> + /* We freed Drawables in the loop above but the underlaying RedDrawables
> + * could still be referenced by Glz so scan again the Glz for independent
> + * drawables. Note that there is a 1-to-1 relatioship between Drawable
> + * and RedDrawable so in theory there will be a maximum of saved_to_free
> + * RedDrawables to free. Use this limit in any case, this should not be
> + * a problem and will make sure we won't free more items that we are
> + * supposed to free. */
> + left_to_free = saved_to_free;
> + FOREACH_DCC(display, iter, dcc) {
> + ImageEncoders *encoders = dcc_get_encoders(dcc);
> + left_to_free -= image_encoders_free_some_independent_glz_drawables(encoders, left_to_free);
> + }
> }
> }
>
> @@ -1285,7 +1292,7 @@ static Drawable *display_channel_drawable_try_new(DisplayChannel *display,
> Drawable *drawable;
>
> while (!(drawable = drawable_try_new(display))) {
> - if (!free_one_drawable(display, FALSE))
> + if (!free_one_drawable(display))
> return NULL;
> }
>
> @@ -1362,7 +1369,7 @@ void drawable_unref(Drawable *drawable)
> drawable_unref_surface_deps(display, drawable);
> display_channel_surface_unref(display, drawable->surface_id);
>
> - glz_retention_detach_drawables(&drawable->glz_retention);
> + glz_retention_destroy(&drawable->glz_retention);
>
> if (drawable->red_drawable) {
> red_drawable_unref(drawable->red_drawable);
A lot of the changes up to now seems to be about removing the second
argument to free_one_drawable(), and renaming "n" to "left_to_free", and
passing RED_RELEASE_BUNCH_SIZE as a parameter to
image_encoders_free_some_independent_glz_drawables() rather than
hardcoding it in that function. This feels not directly related to the
type simplification you are trying to do, is it?
> @@ -1866,9 +1873,8 @@ static void on_disconnect(RedChannelClient *rcc)
> display_channel_compress_stats_print(display);
>
> // this was the last channel client
> - spice_debug("#draw=%d, #glz_draw=%d",
> - display->priv->drawable_count,
> - display->priv->encoder_shared_data.glz_drawable_count);
> + spice_debug("#draw=%d",
> + display->priv->drawable_count);
> }
>
> static int handle_migrate_flush_mark(RedChannelClient *rcc)
> @@ -2117,10 +2123,9 @@ void display_channel_debug_oom(DisplayChannel *display, const char *msg)
> {
> RedChannel *channel = RED_CHANNEL(display);
>
> - spice_debug("%s #draw=%u, #glz_draw=%u current %u pipes %u",
> + spice_debug("%s #draw=%u, current %u pipes %u",
> msg,
> display->priv->drawable_count,
> - display->priv->encoder_shared_data.glz_drawable_count,
> display->priv->current_size,
> red_channel_sum_pipes_size(channel));
> }
Ideally this would go in a follow-up commit, no idea if that's possible.
> diff --git a/server/image-encoders.c b/server/image-encoders.c
> index 39aca6c..951fe96 100644
> --- a/server/image-encoders.c
> +++ b/server/image-encoders.c
> @@ -30,11 +30,6 @@
>
> #define ENCODER_MESSAGE_SIZE 512
>
> -#define MAX_GLZ_DRAWABLE_INSTANCES 2
> -
> -typedef struct RedGlzDrawable RedGlzDrawable;
> -typedef struct GlzDrawableInstanceItem GlzDrawableInstanceItem;
> -
> struct GlzSharedDictionary {
> GlzEncDictContext *dict;
> uint32_t refs;
> @@ -44,37 +39,28 @@ struct GlzSharedDictionary {
> RedClient *client; // channel clients of the same client share the dict
> };
>
> -/* for each qxl drawable, there may be several instances of lz drawables */
> -/* TODO - reuse this stuff for the top level. I just added a second level of multiplicity
> - * at the Drawable by keeping a ring, so:
> - * Drawable -> (ring of) RedGlzDrawable -> (up to 2) GlzDrawableInstanceItem
> - * and it should probably (but need to be sure...) be
> - * Drawable -> ring of GlzDrawableInstanceItem.
> +/**
> + * This structure is used to retain RedDrawable objects so they don't
> + * disappear while we are using their memory.
> + * This is because Glz will keep using image memory for future compressions.
> */
> -struct GlzDrawableInstanceItem {
> - RingItem glz_link;
> +typedef struct GlzDictItem {
> + /** linked to ImageEncoders::glz_dict_items. Always inserted */
> + RingItem glz_context_link;
> + /** linked to GlzImageRetention::ring or ImageEncoders::orphans. Always inserted */
> + RingItem retention_link;
> + /** linked to ImageEncoders::glz_dict_items_to_free */
> RingItem free_link;
For what it's worth, this is the kind of situations where GLists would
make the code much easier to follow, no need to keep track that when
"retention_link" is used, we are changing either GlzImageRetention::ring
or ImageEncoders::orphans, when "glz_context_link" is used, then we are
using ImageEncoders::glz_dict_items and so on. I'm really having a hard
time keeping track of this.
> - GlzEncDictImageContext *context;
> - RedGlzDrawable *glz_drawable;
> -};
> -
> -struct RedGlzDrawable {
> - RingItem link; // ordered by the time it was encoded
> - RingItem drawable_link;
> + /** context pointer from Glz encoder, used to free the drawable */
> + GlzEncDictImageContext *dict_image_context;
> + /** where the drawable came from */
> + ImageEncoders *enc;
> + /** hold a reference to this object, this to make sure Glz
> + * can continue to use its memory */
> RedDrawable *red_drawable;
> - GlzDrawableInstanceItem instances_pool[MAX_GLZ_DRAWABLE_INSTANCES];
> - Ring instances;
> - uint8_t instances_count;
> - gboolean has_drawable;
> - ImageEncoders *encoders;
> -};
> -
> -#define LINK_TO_GLZ(ptr) SPICE_CONTAINEROF((ptr), RedGlzDrawable, \
> - drawable_link)
> -#define DRAWABLE_FOREACH_GLZ_SAFE(drawable, link, next, glz) \
> - SAFE_FOREACH(link, next, drawable, &(drawable)->glz_retention.ring, glz, LINK_TO_GLZ(link))
> +} GlzDictItem;
>
> -static void glz_drawable_instance_item_free(GlzDrawableInstanceItem *instance);
> +static gboolean glz_context_free(GlzDictItem *glz_item);
> static void encoder_data_init(EncoderData *data);
> static void encoder_data_reset(EncoderData *data);
> static void image_encoders_release_glz(ImageEncoders *enc);
> @@ -381,23 +367,24 @@ static void image_encoders_init_lz(ImageEncoders *enc)
> static void glz_usr_free_image(GlzEncoderUsrContext *usr, GlzUsrImageContext *image)
> {
> GlzData *lz_data = (GlzData *)usr;
> - GlzDrawableInstanceItem *glz_drawable_instance = (GlzDrawableInstanceItem *)image;
> - ImageEncoders *drawable_enc = glz_drawable_instance->glz_drawable->encoders;
> + GlzDictItem *glz_item = (GlzDictItem *)image;
> + ImageEncoders *drawable_enc = glz_item->enc;
Here we've removed one level of indirection, but at the same time
glz_drawable_instance was renamed to glz_item, and the 'encoders' member
to 'enc'. This really is not helping with the review, this is "renaming
noise", but one has to be careful to figure this out, and to remember
this through the rest of the changes in that part of the code.
> ImageEncoders *this_enc = SPICE_CONTAINEROF(lz_data, ImageEncoders, glz_data);
> + glz_item->dict_image_context = NULL;
> if (this_enc == drawable_enc) {
> - glz_drawable_instance_item_free(glz_drawable_instance);
> + glz_context_free(glz_item);
> } else {
> /* The glz dictionary is shared between all DisplayChannelClient
> - * instances that belong to the same client, and glz_usr_free_image
> + * items that belong to the same client, and glz_usr_free_image
> * can be called by the dictionary code
> * (glz_dictionary_window_remove_head). Thus this function can be
> * called from any DisplayChannelClient thread, hence the need for
> * this check.
> */
> - pthread_mutex_lock(&drawable_enc->glz_drawables_inst_to_free_lock);
> - ring_add_before(&glz_drawable_instance->free_link,
> - &drawable_enc->glz_drawables_inst_to_free);
> - pthread_mutex_unlock(&drawable_enc->glz_drawables_inst_to_free_lock);
> + pthread_mutex_lock(&drawable_enc->glz_dict_items_to_free_lock);
> + ring_add_before(&glz_item->free_link,
> + &drawable_enc->glz_dict_items_to_free);
> + pthread_mutex_unlock(&drawable_enc->glz_dict_items_to_free_lock);
> }
> }
>
> @@ -456,9 +443,10 @@ void image_encoders_init(ImageEncoders *enc, ImageEncoderSharedData *shared_data
> spice_assert(shared_data);
> enc->shared_data = shared_data;
>
> - ring_init(&enc->glz_drawables);
> - ring_init(&enc->glz_drawables_inst_to_free);
> - pthread_mutex_init(&enc->glz_drawables_inst_to_free_lock, NULL);
> + ring_init(&enc->glz_orphans);
> + ring_init(&enc->glz_dict_items);
> + ring_init(&enc->glz_dict_items_to_free);
> + pthread_mutex_init(&enc->glz_dict_items_to_free_lock, NULL);
More "renaming noise"...
> /*
> - * Remove from the global lz dictionary some glz_drawables that have no reference to
> + * Remove from the global lz dictionary some glz_dict_items that have no reference to
> * Drawable (their qxl drawables are released too).
> - * NOTE - the caller should prevent encoding using the dictionary during the operation
> */
> -int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc)
> +int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc, int max_to_free)
> {
> RingItem *ring_link;
> int n = 0;
>
> - if (!enc) {
> - return 0;
> + if (!enc || !enc->glz_dict || max_to_free <= 0) {
> + return n;
> }
> - ring_link = ring_get_head(&enc->glz_drawables);
> - while ((n < RED_RELEASE_BUNCH_SIZE) && (ring_link != NULL)) {
> - RedGlzDrawable *glz_drawable = SPICE_CONTAINEROF(ring_link, RedGlzDrawable, link);
> - ring_link = ring_next(&enc->glz_drawables, ring_link);
> - if (!glz_drawable->has_drawable) {
> - red_glz_drawable_free(glz_drawable);
> - n++;
> +
> + // prevent encoding using the dictionary during the operation
> + pthread_rwlock_wrlock(&enc->glz_dict->encode_lock);
> + while (max_to_free > n
> + && (ring_link = ring_get_head(&enc->glz_orphans)) != NULL) {
> + GlzDictItem *glz_item = SPICE_CONTAINEROF(ring_link, GlzDictItem, retention_link);
> + if (glz_context_free(glz_item)) {
> + ++n;
> }
> }
> + pthread_rwlock_unlock(&enc->glz_dict->encode_lock);
> return n;
> }
>
> @@ -613,14 +543,12 @@ void image_encoders_free_glz_drawables_to_free(ImageEncoders* enc)
> if (!enc->glz_dict) {
> return;
> }
> - pthread_mutex_lock(&enc->glz_drawables_inst_to_free_lock);
> - while ((ring_link = ring_get_head(&enc->glz_drawables_inst_to_free))) {
> - GlzDrawableInstanceItem *drawable_instance = SPICE_CONTAINEROF(ring_link,
> - GlzDrawableInstanceItem,
> - free_link);
> - glz_drawable_instance_item_free(drawable_instance);
> + pthread_mutex_lock(&enc->glz_dict_items_to_free_lock);
> + while ((ring_link = ring_get_head(&enc->glz_dict_items_to_free))) {
> + GlzDictItem * glz_item = SPICE_CONTAINEROF(ring_link, GlzDictItem, free_link);
> + glz_context_free(glz_item);
And here for example, renaming gets even more in the way. Are
glz_context_free() and glz_drawable_instance_item_free() the same
method? Are they doing something different? Looking at their code, they
sure are different. How much is due to renaming, how much is related to
the merging/some of the design changes you introduced? This really is
difficult for me to review, any changes that this is possible to split?
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel