This is the mail archive of the cygwin-patches mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Re: Cygwin select() issues and improvements


Hi John,

On Mar 19 18:43, john hood wrote:
> From c805552cdc9e673ef2330388ddb8b7a0da741766 Mon Sep 17 00:00:00 2001
> From: John Hood <cgull@glup.org>
> Date: Thu, 28 Jan 2016 17:08:39 -0500
> Subject: [PATCH 1/5] Use high-resolution timebases for select().
> 
> 	* cygwait.h: Add cygwait_us() methods.
> 	* select.h: Change prototype for select_stuff::wait() for larger
> 	microsecond timeouts.
> 	* select.cc (pselect): Convert from old cygwin_select().
> 	Implement microsecond timeouts.
> 	(cygwin_select): Rewrite as a wrapper on pselect().
> 	(select): Implement microsecond timeouts.
> 	(select_stuff::wait): Implement microsecond timeouts with a timer
> 	object.

I have a bit of a problem with patch 1 and 4.  In the same patchset
you add cygwait_us and remove it again.  That doesn't really look
useful to me.  Can you create the patches so that this part is skipped,
please?  The rest of the patch should work as is with the existing version
of cygwait.

Two general style issues:

- Please don't use Microsofts variable naming convention.  It's used in
  kernel.cc to use the same names as in the documentation and there are
  a few rare cases where class members are using this convention, but
  other than that we usually use lowercase and underscores only.  Please
  use that as well.

- Always prepend a space to an opening bracket in function or macro calls,
  gnu-style.  There are a couple of cases where you missed that.  If you find
  such cases prior to your patch, pleae change them while you're at it.

Btw., it would be helpful to get a patch series the way git format-patch/
send-email generates them.  It allows easier review to have every patch
in a single mail.  I changed the text on https://cygwin.com/contrib.html
to be a bit more clear about this.  Well, hopefully a bit more clear.

> @@ -362,13 +362,50 @@ err:
>  /* The heart of select.  Waits for an fd to do something interesting. */
>  select_stuff::wait_states
>  select_stuff::wait (fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
> -		    DWORD ms)
> +		    LONGLONG us)
>  {
>    HANDLE w4[MAXIMUM_WAIT_OBJECTS];
>    select_record *s = &start;
>    DWORD m = 0;
>  
> +  /* Always wait for signals. */
>    wait_signal_arrived here (w4[m++]);
> +
> +  /* Set a timeout, or not, for WMFO. */
> +  DWORD dTimeoutMs;
> +  if (us == 0)
> +    {
> +      dTimeoutMs = 0;
> +    }
> +  else
> +    {
> +      dTimeoutMs = INFINITE;
> +    }

Please, no braces for oneliners.  Also, this entire statement can be
folded into a oneliner:

     ms = us ? INFINITE : 0;

> +  status = NtCreateTimer(&hTimeout, TIMER_ALL_ACCESS, NULL, NotificationTimer);

Does it really make sense to build up and break down a timer per each
call to select_stuff::wait?  This function is called in a loop.  What
about creating the timer in the caller and only arm and disarm it in the
wait call?

> +  if (dTimeoutMs == INFINITE)
> +    {
> +      CancelWaitableTimer (hTimeout);
> +      CloseHandle (hTimeout);
> +    }

For clarity, since the timer has been created and armed using native
functions, please use NtCancelTimer and NtClose here.

> From 225f852594d9ff6a1231063ece3d529b9cc1bf7f Mon Sep 17 00:00:00 2001
> From: John Hood <cgull@glup.org>
> Date: Sat, 30 Jan 2016 17:33:36 -0500
> Subject: [PATCH 2/5] Move get_nonascii_key into fhandler_console.
> 
> 	* fhandler.h (fhandler_console): Move get_nonascii_key() from
> 	select.c into this class.
> 	* select.cc (peek_console): Move get_nonascii_key() into
> 	fhandler_console class.

Patch applied.

> From b2e2b5ac1d6b62610c51a66113e5ab97b1d43750 Mon Sep 17 00:00:00 2001
> From: John Hood <cgull@glup.org>
> Date: Sat, 30 Jan 2016 17:37:33 -0500
> Subject: [PATCH 3/5] Debug printfs.
> 
> 	* fhandler.cc (fhandler_base::get_readahead): Add debug code.
> 	* fhandler_console.cc (fhandler_console::read): Add debug code.
> 	* select.cc (pselect): Add debug code.
> 	(peek_console): Add debug code.

Why?  It's a lot of additional debug output.  Was that only required for
developing or does it serve a real purpose for debugging user bug reports
in future?  If so, I wouldn't mind to have a bit of additional description
in the git log to explain the debug statements...

> From cf2db014fefd4a8488316cf9313325b79e25518d Mon Sep 17 00:00:00 2001
> From: John Hood <cgull@glup.org>
> Date: Thu, 4 Feb 2016 00:44:56 -0500
> Subject: [PATCH 4/5] Improve and simplify select().
> 
> 	* cygwait.h (cygwait_us) Remove; this reverts previous changes.
> 	* select.h: Eliminate redundant select_stuff::select_loop state.
> 	* select.cc (select): Eliminate redundant

See above.

> @@ -182,30 +181,7 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
>  	  }
>        select_printf ("sel.always_ready %d", sel.always_ready);
>  
> -      /* Degenerate case.  No fds to wait for.  Just wait for time to run out
> -	 or signal to arrive. */
> -      if (sel.start.next == NULL)
> -	switch (cygwait_us (us))
> -	  {
> -	  case WAIT_SIGNALED:
> -	    select_printf ("signal received");
> -	    /* select() is always interrupted by a signal so set EINTR,
> -	       unconditionally, ignoring any SA_RESTART detection by
> -	       call_signal_handler().  */
> -	    _my_tls.call_signal_handler ();
> -	    set_sig_errno (EINTR);
> -	    wait_state = select_stuff::select_signalled;
> -	    break;
> -	  case WAIT_CANCELED:
> -	    sel.destroy ();
> -	    pthread::static_cancel_self ();
> -	    /*NOTREACHED*/
> -	  default:
> -	    /* Set wait_state to zero below. */
> -	    wait_state = select_stuff::select_set_zero;
> -	    break;
> -	  }
> -      else if (sel.always_ready || us == 0)

This obviously allows to fold everything into select_stuff::wait, but
the more it seems like a good idea to move the timer creation into the
caller for this case, doesn't it?

Alternatively, we could add a per-thread timer handle to the cygtls
area.  It could be created on first use and deleted when the thread
exits.  But that's just an idea for a future improvement, never
mind for now.

> From 3f3f7112f948d70c15046641cf3cc898a9ca4c71 Mon Sep 17 00:00:00 2001
> From: John Hood <cgull@glup.org>
> Date: Fri, 18 Mar 2016 04:31:16 -0400
> Subject: [PATCH 5/5] 	* winsup/testsuite/configure: chmod a+x

Applied.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: signature.asc
Description: PGP signature


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]