Re: [Spice-devel] [PATCH] Do not loop on ERESTARTSYS using interruptible waits

[Christophe Fergeau]

Hey,

On Fri, May 22, 2015 at 09:58:10AM -0400, Frediano Ziglio wrote:
> > >  }
> > >  
> > >  int qxl_io_update_area(struct qxl_device *qdev, struct qxl_bo *surf,
> > > -			const struct qxl_rect *area)
> > > +			const struct qxl_rect *area, bool intr)
> > >  {
> > >  	int surface_id;
> > >  	uint32_t surface_width, surface_height;
> > > @@ -350,7 +347,7 @@ int qxl_io_update_area(struct qxl_device *qdev, struct
> > > qxl_bo *surf,
> > >  	mutex_lock(&qdev->update_area_mutex);
> > >  	qdev->ram_header->update_area = *area;
> > >  	qdev->ram_header->update_surface = surface_id;
> > > -	ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, true);
> > > +	ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, intr);
> > >  	mutex_unlock(&qdev->update_area_mutex);
> > >  	return ret;
> > >  }
> > > @@ -588,10 +585,7 @@ int qxl_update_surface(struct qxl_device *qdev, struct
> > > qxl_bo *surf)
> > >  	rect.right = surf->surf.width;
> > >  	rect.top = 0;
> > >  	rect.bottom = surf->surf.height;
> > > -retry:
> > > -	ret = qxl_io_update_area(qdev, surf, &rect);
> > > -	if (ret == -ERESTARTSYS)
> > > -		goto retry;
> > > +	ret = qxl_io_update_area(qdev, surf, &rect, false);
> > 
> > My understanding is that the fix is this hunk? If so, this could be made
> > more obvious with an intermediate commit adding the 'bool intr' arg to
> > qxl_io_update_area and only calling it with 'true' in the appropriate
> > places.
> > This code path is only triggered from qxl_surface_evict() which I assume
> > is not necessarily easily interruptible, so this change makes sense to
> > me. However it would be much better to get a review from Dave Airlie ;)
> > 
> > Christophe
> > 
> 
> Are you asking if just removing the loop would fix the issue?
> So you are proposing a first patch that add the argument always
> passing true and another that change some calls to false? It make
> sense but still the loop should be removed.

Sorry, I was not clear, removing the loop is fine, I was not suggesting
to keep it.

Christophe

Attachment: pgpMftIo4G0Nu.pgp
Description: PGP signature