subject:"Re\: \[PATCH xserver 2\/6\] Remove SIGIO support."

Re: [PATCH xserver 2/6] Remove SIGIO support.

2015-12-09 Thread Keith Packard

Peter Hutterer  writes:

>>  /* Call PIE here so we don't try to dereference a device that's
>>   * already been removed. */
>> -OsBlockSignals();
>>  ProcessInputEvents();
>> +input_lock();
>
> this is a behaviour change, was this intended? I'd rather not have this
> hidden in a giant patch that is otherwise mostly search and replace.

(I knew locking was the hard part...).

OsBlockSignals has a counter, so you can call it multiple
times. input_lock is (going to be) a regular mutex, so you can't (and
ProcessInputEvents definitely needs to hold the input lock). I moved
this to eliminate a deadlock, only to now discover that xfree86's
DeleteInputDeviceRequest grabs the input lock itself. And, kdrive's
evdev driver calls DeleteInputDeviceRequest from the event reading code (!).

We may uncover more deadlocks in the code; I think each one in isolation
should be pretty easy to fix. We could also implement a counter in the
mutex? That would require using thread local storage, which is kinda
ugly, but should at least allow the existing locking to not deadlock.

I'd rather work on annotating the code and data structures so that we
know when the lock should be held; right now it's somewhat haphazard,
with things like device property changes not taking the lock.

For instance, in this case we would mark the device as 'pending
deletion' and have the event queuing code discard incoming events:

input_lock();
dev->pending_deletion = TRUE;
input_unlock();
ProcessInputEvents();
input_lock();
DeleteInputDeviceRequest(dev);
input_unlock();

>> @@ -1077,8 +1077,8 @@ TouchEndPhysicallyActiveTouches(DeviceIntPtr dev)
>>  InternalEvent *eventlist = InitEventList(GetMaximumEventsNum());
>>  int i;
>>  
>> -OsBlockSignals();
>>  mieqProcessInputEvents();
>> +input_lock();
>
> same here. How about making the first patch all the search/replace cases and
> let input_lock/unlock call OsBlockSignals/OsReleaseSignals. Then in the
> second patch you can change the places where it needs to be moved around,
> then in a third patch actually drop the signal handling.

For this case, mieqProcessDeviceEvent must not be called with the
input_lock held as it (eventually) may call SetCursorPosition, which
will grab input_lock to warp the cursor on the screen. Given what this
function does, it seems like it is equivalent to:

input_lock();
for (i = 0; i < dev->last.num_touches; i++) {
DDXTouchPointInfoPtr ddxti = dev->last.touches + i;

if (ddxti->active)
QueueTouchEvents(dev, XI_TouchEnd, ddxti->ddx_id, 0, NULL);
}
input_unlock();
ProcessInputEvents();

Similarly, ReleaseButtonsAndKeys should be changed to queue the relevant
events under the lock and then process them. That's a bit harder as that
function is looking at the logical state of the device, not the physical
state. It looks like that's just the tip of a large bug though -- a
frozen device that gets removed is going to have events in the
syncEvents list still pointing at it. That seems bad...

>> +xf86ReadInput(int fd, int ready, void *closure) {
>
> new line for {, no empty line before static void

Thanks.

>> diff --git a/hw/xfree86/common/xf86Helper.c b/hw/xfree86/common/xf86Helper.c
>> index c42e93e..b506338 100644
>> --- a/hw/xfree86/common/xf86Helper.c
>> +++ b/hw/xfree86/common/xf86Helper.c
>> @@ -1729,7 +1729,7 @@ xf86SetSilkenMouse(ScreenPtr pScreen)
>>   * yet.  Should handle this differently so that alternate async methods
>>   * work correctly with this too.
>>   */
>> -pScrn->silkenMouse = useSM && xf86Info.useSIGIO && xf86SIGIOSupported();
>> +pScrn->silkenMouse = useSM && FALSE;
>
> wait, what?

It's a temporary hack -- having removed SIGIO and not replaced it with
threading, all of the 'silken mouse' code can't actually work. Once we
add threads, then this looks sensible again.

>>  libbsd_la_SOURCES = \
>>  $(srcdir)/../shared/posix_tty.c \
>> -$(srcdir)/../shared/sigio.c \
>>  $(srcdir)/../shared/vidmem.c \
>>  bsd_VTsw.c \
>>  bsd_init.c \
>
> [...]

I split the DRI1 breakage and SIGIO API removal out into separate
patches so we can discuss whether DRI1 needs to be supported independently
of whether input should be threaded.

>> diff --git a/xkb/xkbActions.c b/xkb/xkbActions.c
>> index aeb702c..effda7c 100644
>> --- a/xkb/xkbActions.c
>> +++ b/xkb/xkbActions.c
>> @@ -1526,7 +1526,7 @@ InjectPointerKeyEvents(DeviceIntPtr dev, int type, int 
>> button, int flags,
>>  return;
>>  
>>  events = InitEventList(GetMaximumEventsNum() + 1);
>> -OsBlockSignals();
>> +input_lock();
>>  pScreen = miPointerGetScreen(ptr);
>>  saveWait = miPointerSetWaitForUpdate(pScreen, FALSE);
>>  nevents = GetPointerEvents(events, ptr, type, button, flags, mask);
>> @@ -1534,10 +1534,10 @@ InjectPointerKeyEvents(DeviceIntPtr dev, int type, 
>> int

Re: [PATCH xserver 2/6] Remove SIGIO support.

2015-12-09 Thread Peter Hutterer

On Wed, Dec 09, 2015 at 11:13:14AM -0800, Keith Packard wrote:
> Peter Hutterer  writes:
> 
> >>  /* Call PIE here so we don't try to dereference a device that's
> >>   * already been removed. */
> >> -OsBlockSignals();
> >>  ProcessInputEvents();
> >> +input_lock();
> >
> > this is a behaviour change, was this intended? I'd rather not have this
> > hidden in a giant patch that is otherwise mostly search and replace.
> 
> (I knew locking was the hard part...).
> 
> OsBlockSignals has a counter, so you can call it multiple
> times. input_lock is (going to be) a regular mutex, so you can't (and
> ProcessInputEvents definitely needs to hold the input lock). I moved
> this to eliminate a deadlock, only to now discover that xfree86's
> DeleteInputDeviceRequest grabs the input lock itself. And, kdrive's
> evdev driver calls DeleteInputDeviceRequest from the event reading code (!).
> 
> We may uncover more deadlocks in the code; I think each one in isolation
> should be pretty easy to fix. We could also implement a counter in the
> mutex? That would require using thread local storage, which is kinda
> ugly, but should at least allow the existing locking to not deadlock.
> 
> I'd rather work on annotating the code and data structures so that we
> know when the lock should be held; right now it's somewhat haphazard,
> with things like device property changes not taking the lock.
> 
> For instance, in this case we would mark the device as 'pending
> deletion' and have the event queuing code discard incoming events:
> 
> input_lock();
> dev->pending_deletion = TRUE;
> input_unlock();
> ProcessInputEvents();
> input_lock();
> DeleteInputDeviceRequest(dev);
> input_unlock();
> 
> >> @@ -1077,8 +1077,8 @@ TouchEndPhysicallyActiveTouches(DeviceIntPtr dev)
> >>  InternalEvent *eventlist = InitEventList(GetMaximumEventsNum());
> >>  int i;
> >>  
> >> -OsBlockSignals();
> >>  mieqProcessInputEvents();
> >> +input_lock();
> >
> > same here. How about making the first patch all the search/replace cases and
> > let input_lock/unlock call OsBlockSignals/OsReleaseSignals. Then in the
> > second patch you can change the places where it needs to be moved around,
> > then in a third patch actually drop the signal handling.
> 
> For this case, mieqProcessDeviceEvent must not be called with the
> input_lock held as it (eventually) may call SetCursorPosition, which
> will grab input_lock to warp the cursor on the screen. 

yes, but you can do that with the suggestion above. do a straightforward
search/replace for OsBlockSignals -> input_lock, then when you drop 
OsBlockSignals from input_lock you can move things where needed. plus you
get to see the places where the reordering was needed stand out.
[ok, now that I read the rest of the email I see you're suggesting to do
this further below]

> Given what this
> function does, it seems like it is equivalent to:
> 
> input_lock();
> for (i = 0; i < dev->last.num_touches; i++) {
> DDXTouchPointInfoPtr ddxti = dev->last.touches + i;
> 
> if (ddxti->active)
> QueueTouchEvents(dev, XI_TouchEnd, ddxti->ddx_id, 0, NULL);
> }
> input_unlock();
> ProcessInputEvents();
> 
> Similarly, ReleaseButtonsAndKeys should be changed to queue the relevant
> events under the lock and then process them. That's a bit harder as that
> function is looking at the logical state of the device, not the physical
> state. It looks like that's just the tip of a large bug though -- a
> frozen device that gets removed is going to have events in the
> syncEvents list still pointing at it. That seems bad...
> 
> >> +xf86ReadInput(int fd, int ready, void *closure) {
> >
> > new line for {, no empty line before static void
> 
> Thanks.
> 
> >> diff --git a/hw/xfree86/common/xf86Helper.c 
> >> b/hw/xfree86/common/xf86Helper.c
> >> index c42e93e..b506338 100644
> >> --- a/hw/xfree86/common/xf86Helper.c
> >> +++ b/hw/xfree86/common/xf86Helper.c
> >> @@ -1729,7 +1729,7 @@ xf86SetSilkenMouse(ScreenPtr pScreen)
> >>   * yet.  Should handle this differently so that alternate async 
> >> methods
> >>   * work correctly with this too.
> >>   */
> >> -pScrn->silkenMouse = useSM && xf86Info.useSIGIO && 
> >> xf86SIGIOSupported();
> >> +pScrn->silkenMouse = useSM && FALSE;
> >
> > wait, what?
> 
> It's a temporary hack -- having removed SIGIO and not replaced it with
> threading, all of the 'silken mouse' code can't actually work. Once we
> add threads, then this looks sensible again.

add a comment please, if for no other reason than to reduce confusion of
those who look at the patches separately.

> 
> >>  libbsd_la_SOURCES = \
> >>$(srcdir)/../shared/posix_tty.c \
> >> -  $(srcdir)/../shared/sigio.c \
> >>$(srcdir)/../shared/vidmem.c \
> >>bsd_VTsw.c \
> >>bsd_init.c \
> >
> > [...]
> 
> I split the DRI1 breakage and SIGIO API

Re: [PATCH xserver 2/6] Remove SIGIO support.

2015-12-09 Thread Keith Packard

Peter Hutterer  writes:

> this is a behaviour change, was this intended? I'd rather not have this
> hidden in a giant patch that is otherwise mostly search and replace.

Ok, I've pushed out a new series which creates a recursive (counting)
mutex that makes all of this stuff quite easy. Nice to have only two
threads, and to not be worrying about performance here. In particular, I
didn't have to move locks around, except for one case where the existing
code looked like it wasn't locking enough. That's a separate patch.

I'll send the updated series to the list; it's also in my git repository
at:

git://people.freedesktop.org/~keithp/xserver.git input-thread

This also removes the (controversial) DRI1 changes from this series;
they're tacked ontop of the fd_set patches in my remove-xfree86-sigio
branch.

-- 
-keith

signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 2/6] Remove SIGIO support.

2015-12-08 Thread Alan Coopersmith


On 12/ 8/15 03:44 PM, Keith Packard wrote:

This removes all of the SIGIO handling support throughout the X
server, preparing the way for using threads for input handling
instead.

Places calling OsBlockSIGIO and OsReleaseSIGIO are marked with calls
to stub functions input_lock/input_unlock so that we don't lose this
information.

Signed-off-by: Keith Packard 
---
  Xi/exevents.c |   4 +-
  config/config.c   |   4 +-
  dix/devices.c |  10 +-
  dix/ptrveloc.c|   4 +-
  dix/touch.c   |   8 +-
  hw/dmx/input/dmxevents.c  |  24 +--
  hw/kdrive/ephyr/ephyr.c   |   4 +-
  hw/kdrive/src/kinput.c|  74 +---
  hw/xfree86/common/xf86Config.c|  24 ---
  hw/xfree86/common/xf86Cursor.c|   8 +-
  hw/xfree86/common/xf86Events.c|  33 ++--
  hw/xfree86/common/xf86Helper.c|   2 +-
  hw/xfree86/common/xf86Init.c  |   6 +-
  hw/xfree86/common/xf86PM.c|   8 +-
  hw/xfree86/common/xf86Xinput.c|   8 +-
  hw/xfree86/dri/dri.c  |  54 +-
  hw/xfree86/os-support/bsd/Makefile.am |   1 -
  hw/xfree86/os-support/hurd/Makefile.am|   1 -
  hw/xfree86/os-support/linux/Makefile.am   |   1 -
  hw/xfree86/os-support/shared/sigio.c  | 291 --
  hw/xfree86/os-support/shared/sigiostubs.c |  70 ---
  hw/xfree86/os-support/solaris/Makefile.am |   1 -
  hw/xfree86/os-support/stub/Makefile.am|   1 -
  hw/xfree86/os-support/xf86_OSproc.h   |  12 --
  include/os.h  |   6 -
  mi/mieq.c |   6 +-
  mi/mipointer.c|   4 +-
  os/utils.c|  54 +-
  test/Makefile.am  |   2 +-
  test/os.c | 166 -
  xkb/xkbActions.c  |   4 +-
  31 files changed, 77 insertions(+), 818 deletions(-)
  delete mode 100644 hw/xfree86/os-support/shared/sigio.c
  delete mode 100644 hw/xfree86/os-support/shared/sigiostubs.c
  delete mode 100644 test/os.c



Shouldn't this also remove all the USE_SIGIO_BY_DEFAULT references in 
configure.ac?

--
-Alan Coopersmith-  alan.coopersm...@oracle.com
 Oracle Solaris Engineering - http://blogs.oracle.com/alanc
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 2/6] Remove SIGIO support.

2015-12-08 Thread Keith Packard

Alan Coopersmith  writes:

> Shouldn't this also remove all the USE_SIGIO_BY_DEFAULT references in
> configure.ac?

Yes. Thanks!

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 2/6] Remove SIGIO support.

2015-12-08 Thread Peter Hutterer

On Tue, Dec 08, 2015 at 03:44:50PM -0800, Keith Packard wrote:
> This removes all of the SIGIO handling support throughout the X
> server, preparing the way for using threads for input handling
> instead.
> 
> Places calling OsBlockSIGIO and OsReleaseSIGIO are marked with calls
> to stub functions input_lock/input_unlock so that we don't lose this
> information.
> 
> Signed-off-by: Keith Packard 

[...]
 
>  /**
> diff --git a/config/config.c b/config/config.c
> index de45cc3..263982e 100644
> --- a/config/config.c
> +++ b/config/config.c
> @@ -86,10 +86,10 @@ remove_device(const char *backend, DeviceIntPtr dev)
>  
>  /* Call PIE here so we don't try to dereference a device that's
>   * already been removed. */
> -OsBlockSignals();
>  ProcessInputEvents();
> +input_lock();

this is a behaviour change, was this intended? I'd rather not have this
hidden in a giant patch that is otherwise mostly search and replace.

>  DeleteInputDeviceRequest(dev);
> -OsReleaseSignals();
> +input_unlock();
>  }
>  
>  void
> diff --git a/dix/devices.c b/dix/devices.c
> index 9b0c7d2..7ba6b94 100644
> --- a/dix/devices.c
> +++ b/dix/devices.c

[...]

> @@ -1077,8 +1077,8 @@ TouchEndPhysicallyActiveTouches(DeviceIntPtr dev)
>  InternalEvent *eventlist = InitEventList(GetMaximumEventsNum());
>  int i;
>  
> -OsBlockSignals();
>  mieqProcessInputEvents();
> +input_lock();

same here. How about making the first patch all the search/replace cases and
let input_lock/unlock call OsBlockSignals/OsReleaseSignals. Then in the
second patch you can change the places where it needs to be moved around,
then in a third patch actually drop the signal handling.

>  for (i = 0; i < dev->last.num_touches; i++) {
>  DDXTouchPointInfoPtr ddxti = dev->last.touches + i;
>  
> @@ -1091,7 +1091,7 @@ TouchEndPhysicallyActiveTouches(DeviceIntPtr dev)
>  mieqProcessDeviceEvent(dev, eventlist + j, NULL);
>  }
>  }
> -OsReleaseSignals();
> +input_unlock();
>  
>  FreeEventList(eventlist, GetMaximumEventsNum());
>  }
> diff --git a/hw/dmx/input/dmxevents.c b/hw/dmx/input/dmxevents.c
> index 2b579ee..3789602 100644
> --- a/hw/dmx/input/dmxevents.c
> +++ b/hw/dmx/input/dmxevents.c
> @@ -227,25 +227,25 @@ dmxCoreMotion(DevicePtr pDev, int x, int y, int delta, 
> DMXBlockType block)
>  && pScreen->myNum == dmxScreen->index) {
>  /* Screen is old screen */
>  if (block)
> -OsBlockSIGIO();
> +input_lock();
>  if (pDev)
>  enqueueMotion(pDev, localX, localY);
>  if (block)
> -OsReleaseSIGIO();
> +input_unlock();
>  }
>  else {
>  /* Screen is new */
>  DMXDBG4("   New screen: old=%d new=%d localX=%d localY=%d\n",
>  pScreen->myNum, dmxScreen->index, localX, localY);
>  if (block)
> -OsBlockSIGIO();
> +input_lock();
>  mieqProcessInputEvents();
>  miPointerSetScreen(inputInfo.pointer, dmxScreen->index,
> localX, localY);
>  if (pDev)
>  enqueueMotion(pDev, localX, localY);
>  if (block)
> -OsReleaseSIGIO();
> +input_unlock();
>  }
>  #if 00
>  miPointerGetPosition(inputInfo.pointer, , );
> @@ -387,12 +387,12 @@ dmxExtMotion(DMXLocalInputInfoPtr dmxLocal,
>  }
>  
>  if (block)
> -OsBlockSIGIO();
> +input_lock();
>  valuator_mask_set_range(, firstAxis, axesCount, v);
>  QueuePointerEvents(pDevice, MotionNotify, 0, POINTER_ABSOLUTE, );
>  
>  if (block)
> -OsReleaseSIGIO();
> +input_unlock();
>  }
>  
>  static int
> @@ -489,10 +489,10 @@ dmxTranslateAndEnqueueExtEvent(DMXLocalInputInfoPtr 
> dmxLocal,
>  case XI_DeviceKeyPress:
>  case XI_DeviceKeyRelease:
>  if (block)
> -OsBlockSIGIO();
> +input_lock();
>  QueueKeyboardEvents(pDevice, event, ke->keycode);
>  if (block)
> -OsReleaseSIGIO();
> +input_unlock();
>  break;
>  case XI_DeviceButtonPress:
>  case XI_DeviceButtonRelease:
> @@ -500,11 +500,11 @@ dmxTranslateAndEnqueueExtEvent(DMXLocalInputInfoPtr 
> dmxLocal,
>  valuator_mask_set_range(, ke->first_axis, ke->axes_count,
>  valuators);
>  if (block)
> -OsBlockSIGIO();
> +input_lock();
>  QueuePointerEvents(pDevice, event, ke->keycode,
> POINTER_ABSOLUTE, );
>  if (block)
> -OsReleaseSIGIO();
> +input_unlock();
>  break;
>  case XI_ProximityIn:
>  case XI_ProximityOut:
> @@ -512,10 +512,10 @@ dmxTranslateAndEnqueueExtEvent(DMXLocalInputInfoPtr 
> dmxLocal,
>

Re: [PATCH xserver 2/6] Remove SIGIO support.

Re: [PATCH xserver 2/6] Remove SIGIO support.

Re: [PATCH xserver 2/6] Remove SIGIO support.

Re: [PATCH xserver 2/6] Remove SIGIO support.

Re: [PATCH xserver 2/6] Remove SIGIO support.

Re: [PATCH xserver 2/6] Remove SIGIO support.

6 matches

Site Navigation

Mail list logo

Footer information