httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@attglobal.net>
Subject Re: [PATCH] AcceptMutex and SingleListen runtime
Date Wed, 22 Aug 2001 17:53:29 GMT
Jim Jagielski <jim@jaguNET.com> writes:

> Comments are welcome... I'd like to commit the patch this week. At
> that point, we port to 2.0, which already has a lot of the
> work done (just need to do the default stuff and handle SingleListen)

which "default stuff" is needed in 2.0?

why is SingleListen needed?

> Index: src/include/ap_config.h
> ===================================================================
> RCS file: /home/cvs/apache-1.3/src/include/ap_config.h,v
> retrieving revision 1.309
> diff -u -r1.309 ap_config.h
> --- src/include/ap_config.h	2001/06/22 12:43:53	1.309
> +++ src/include/ap_config.h	2001/08/20 18:19:42
> @@ -179,10 +179,10 @@
>  #define NO_KILLPG
>  #undef NO_SETSID
>  #define bzero(a,b) memset(a,0,b)
> -#if !defined(USE_SYSVSEM_SERIALIZED_ACCEPT) && \
> -    !defined(USE_PTHREAD_SERIALIZED_ACCEPT)
> +#define USE_SYSVSEM_SERIALIZED_ACCEPT
> +#define USE_PTHREAD_SERIALIZED_ACCEPT

Doesn't HAVE_foo_SERIALIZED_ACCEPT make more sense than
USE_foo_SERIALIZED_ACCEPT?  I guess changing the meaning of
USE_foo_SERIALIZED_ACCEPT helps minimize the changes.


>  #define USE_FCNTL_SERIALIZED_ACCEPT
> -#endif
> +#define DEFAULT_SERIALIZED_ACCEPT_METHOD fcntl

I guess we only have to set DEFAULT_SERIALIZED_ACCEPT_METHOD on
systems where there is more than one choice?

>  #define NEED_UNION_SEMUN
>  #define HAVE_MMAP 1
>  #define USE_MMAP_SCOREBOARD
> @@ -433,6 +433,9 @@
>  #define JMP_BUF jmp_buf
>  #define USE_LONGJMP
>  #define USE_FLOCK_SERIALIZED_ACCEPT
> +#define USE_FCNTL_SERIALIZED_ACCEPT
> +#define USE_NONE_SERIALIZED_ACCEPT
> +#define DEFAULT_SERIALIZED_ACCEPT_METHOD flock
>  #define SINGLE_LISTEN_UNSERIALIZED_ACCEPT

So what decides when "USE_NONE_SERIALIZED_ACCEPT" is a choice?  I'd
think that if you allow it anywhere you'd allow it everywhere, and
that there'd be no need to define anything in ap_config.h.

>  #elif defined(LINUX)

I guess we need more defines here, and on various platforms.  What is
the philosophy?  We could do some test compiles or just leave it like
this, and anybody that gives a s%^& can add #define USE_foo for their
favorite platforms to make foo available.

> Index: src/include/http_main.h
> ===================================================================
> RCS file: /home/cvs/apache-1.3/src/include/http_main.h,v
> retrieving revision 1.36
> diff -u -r1.36 http_main.h
> --- src/include/http_main.h	2001/01/15 17:04:34	1.36
> +++ src/include/http_main.h	2001/08/20 18:19:42
> @@ -130,6 +130,11 @@
>  
>  void setup_signal_names(char *prefix);
>  
> +/* functions for determination and setting of accept() mutexing */
> +char *default_mutex_method(void);
> +char *init_mutex_method(char *t);
> +char *init_single_listen(int flag);

prefix with "ap_"...  yeah, not everything in 1.3 is
namespace-protected, but it is trivial for new functions.

> Index: src/main/http_core.c
> ===================================================================
> RCS file: /home/cvs/apache-1.3/src/main/http_core.c,v
> retrieving revision 1.296
> diff -u -r1.296 http_core.c
> --- src/main/http_core.c	2001/07/13 07:32:44	1.296
> +++ src/main/http_core.c	2001/08/20 18:19:51
> @@ -1135,6 +1135,14 @@
>      }
>      return NULL;
>  }
> +static const char *set_accept_mutex(cmd_parms *cmd, void *dummy, char *arg)
> +{
> +	return init_mutex_method(arg);
> +}
> +static const char *set_single_listen(cmd_parms *cmd, void *dummy, int flag)
> +{
> +	return init_single_listen(flag ? 1 : 0);
> +}
>  
>  static const char *set_document_root(cmd_parms *cmd, void *dummy, char *arg)
>  {
> @@ -3215,6 +3223,37 @@
>    (void*)XtOffsetOf(core_dir_config, limit_req_body),
>    OR_ALL, TAKE1,
>    "Limit (in bytes) on maximum size of request message body" },
> +{ "AcceptMutex", set_accept_mutex, NULL, RSRC_CONF, TAKE1,
> +  "Serialized Accept Mutex; the methods "
> +#ifdef USE_FCNTL_SERIALIZED_ACCEPT
> +	"'fcntl' "
> +#endif
> +#ifdef USE_FLOCK_SERIALIZED_ACCEPT
> +	"'flock' "
> +#endif
> +#ifdef USE_USLOCK_SERIALIZED_ACCEPT
> +	"'uslock "
> +#endif
> +#ifdef USE_SYSVSEM_SERIALIZED_ACCEPT
> +	"'sysvsem' "
> +#endif      
> +#ifdef USE_PTHREAD_SERIALIZED_ACCEPT
> +	"'pthread' "
> +#endif
> +#ifdef USE_OS2SEM_SERIALIZED_ACCEPT
> +	"'os2sem' "
> +#endif  
> +#ifdef USE_TPF_CORE_SERIALIZED_ACCEPT
> +	"'tpfcore' "
> +#endif    
> +#ifdef USE_NONE_SERIALIZED_ACCEPT
> +	"'none' "
> +#endif 
> +	"are compiled in"
> +},
> +{ "SingleListen",set_single_listen,NULL,RSRC_CONF,FLAG,
> +	"On - to serialize accept and Off  to unserialize accept"
> +},
>  
>  /* EBCDIC Conversion directives: */
>  #ifdef CHARSET_EBCDIC
> Index: src/main/http_main.c
> ===================================================================
> RCS file: /home/cvs/apache-1.3/src/main/http_main.c,v
> retrieving revision 1.546
> diff -u -r1.546 http_main.c
> --- src/main/http_main.c	2001/08/10 01:14:29	1.546
> +++ src/main/http_main.c	2001/08/20 18:20:05
> @@ -258,6 +258,17 @@
>  time_t ap_restart_time=0;
>  API_VAR_EXPORT int ap_suexec_enabled = 0;
>  int ap_listenbacklog=0;
> +/* On some architectures it's safe to do unserialized accept()s in the single
> + * Listen case.  But it's never safe to do it in the case where there's
> + * multiple Listen statements.  Define SINGLE_LISTEN_UNSERIALIZED_ACCEPT
> + * when it's safe in the single Listen case.
> + */
> +#ifdef SINGLE_LISTEN_UNSERIALIZED_ACCEPT
> +int ap_single_listen = 1;
> +#else
> +int ap_single_listen = 0;
> +#endif
> +
>  #ifdef SO_ACCEPTFILTER
>  int ap_acceptfilter =
>  #ifdef AP_ACCEPTFILTER_OFF
> @@ -513,14 +524,12 @@
>  #endif
>  
>  #if defined (USE_USLOCK_SERIALIZED_ACCEPT)
> -
>  #include <ulocks.h>
> -
>  static ulock_t uslock = NULL;
>  
> -#define accept_mutex_child_init(x)
> +#define accept_mutex_child_init_uslock(x)
>  
> -static void accept_mutex_init(pool *p)
> +static void accept_mutex_init_uslock(pool *p)
>  {
>      ptrdiff_t old;
>      usptr_t *us;
> @@ -551,7 +560,7 @@
>      }
>  }
>  
> -static void accept_mutex_on(void)
> +static void accept_mutex_on_uslock(void)
>  {
>      switch (ussetlock(uslock)) {
>      case 1:
> @@ -566,15 +575,16 @@
>      }
>  }
>  
> -static void accept_mutex_off(void)
> +static void accept_mutex_off_uslock(void)
>  {
>      if (usunsetlock(uslock) == -1) {
>  	perror("usunsetlock");
>  	clean_child_exit(APEXIT_CHILDFATAL);
>      }
>  }
> +#endif
>  
> -#elif defined (USE_PTHREAD_SERIALIZED_ACCEPT)
> +#if defined (USE_PTHREAD_SERIALIZED_ACCEPT)
>  
>  /* This code probably only works on Solaris ... but it works really fast
>   * on Solaris.  Note that pthread mutexes are *NOT* released when a task
> @@ -589,7 +599,7 @@
>  static sigset_t accept_block_mask;
>  static sigset_t accept_previous_mask;
>  
> -static void accept_mutex_child_cleanup(void *foo)
> +static void accept_mutex_child_cleanup_pthread(void *foo)
>  {
>      if (accept_mutex != (void *)(caddr_t)-1
>  	&& have_accept_mutex) {
> @@ -597,12 +607,12 @@
>      }
>  }
>  
> -static void accept_mutex_child_init(pool *p)
> +static void accept_mutex_child_init_pthread(pool *p)
>  {
> -    ap_register_cleanup(p, NULL, accept_mutex_child_cleanup, ap_null_cleanup);
> +    ap_register_cleanup(p, NULL, accept_mutex_child_cleanup_pthread, ap_null_cleanup);
>  }
>  
> -static void accept_mutex_cleanup(void *foo)
> +static void accept_mutex_cleanup_pthread(void *foo)
>  {
>      if (accept_mutex != (void *)(caddr_t)-1
>  	&& munmap((caddr_t) accept_mutex, sizeof(*accept_mutex))) {
> @@ -611,7 +621,7 @@
>      accept_mutex = (void *)(caddr_t)-1;
>  }
>  
> -static void accept_mutex_init(pool *p)
> +static void accept_mutex_init_pthread(pool *p)
>  {
>      pthread_mutexattr_t mattr;
>      int fd;
> @@ -645,10 +655,10 @@
>      sigdelset(&accept_block_mask, SIGHUP);
>      sigdelset(&accept_block_mask, SIGTERM);
>      sigdelset(&accept_block_mask, SIGUSR1);
> -    ap_register_cleanup(p, NULL, accept_mutex_cleanup, ap_null_cleanup);
> +    ap_register_cleanup(p, NULL, accept_mutex_cleanup_pthread, ap_null_cleanup);
>  }
>  
> -static void accept_mutex_on(void)
> +static void accept_mutex_on_pthread(void)
>  {
>      int err;
>  
> @@ -670,7 +680,7 @@
>      ap_unblock_alarms();
>  }
>  
> -static void accept_mutex_off(void)
> +static void accept_mutex_off_pthread(void)
>  {
>      int err;
>  
> @@ -691,8 +701,9 @@
>  	clean_child_exit(1);
>      }
>  }
> +#endif
>  
> -#elif defined (USE_SYSVSEM_SERIALIZED_ACCEPT)
> +#if defined (USE_SYSVSEM_SERIALIZED_ACCEPT)
>  
>  #include <sys/types.h>
>  #include <sys/ipc.h>
> @@ -716,7 +727,7 @@
>   * means we have to be sure to clean this up or else we'll leak
>   * semaphores.
>   */
> -static void accept_mutex_cleanup(void *foo)
> +static void accept_mutex_cleanup_sysvsem(void *foo)
>  {
>      union semun ick;
>  
> @@ -727,9 +738,9 @@
>      semctl(sem_id, 0, IPC_RMID, ick);
>  }
>  
> -#define accept_mutex_child_init(x)
> +#define accept_mutex_child_init_sysvsem(x)
>  
> -static void accept_mutex_init(pool *p)
> +static void accept_mutex_init_sysvsem(pool *p)
>  {
>      union semun ick;
>      struct semid_ds buf;
> @@ -758,7 +769,7 @@
>  	    exit(APEXIT_INIT);
>  	}
>      }
> -    ap_register_cleanup(p, NULL, accept_mutex_cleanup, ap_null_cleanup);
> +    ap_register_cleanup(p, NULL, accept_mutex_cleanup_sysvsem, ap_null_cleanup);
>  
>      /* pre-initialize these */
>      op_on.sem_num = 0;
> @@ -769,7 +780,7 @@
>      op_off.sem_flg = SEM_UNDO;
>  }
>  
> -static void accept_mutex_on(void)
> +static void accept_mutex_on_sysvsem(void)
>  {
>      while (semop(sem_id, &op_on, 1) < 0) {
>  	if (errno != EINTR) {
> @@ -779,7 +790,7 @@
>      }
>  }
>  
> -static void accept_mutex_off(void)
> +static void accept_mutex_off_sysvsem(void)
>  {
>      while (semop(sem_id, &op_off, 1) < 0) {
>  	if (errno != EINTR) {
> @@ -788,20 +799,21 @@
>  	}
>      }
>  }
> +#endif
>  
> -#elif defined(USE_FCNTL_SERIALIZED_ACCEPT)
> +#if defined(USE_FCNTL_SERIALIZED_ACCEPT)
>  static struct flock lock_it;
>  static struct flock unlock_it;
>  
>  static int lock_fd = -1;
>  
> -#define accept_mutex_child_init(x)
> +#define accept_mutex_child_init_fcntl(x)
>  
>  /*
>   * Initialize mutex lock.
>   * Must be safe to call this on a restart.
>   */
> -static void accept_mutex_init(pool *p)
> +static void accept_mutex_init_fcntl(pool *p)
>  {
>  
>      lock_it.l_whence = SEEK_SET;	/* from current point */
> @@ -825,7 +837,7 @@
>      unlink(ap_lock_fname);
>  }
>  
> -static void accept_mutex_on(void)
> +static void accept_mutex_on_fcntl(void)
>  {
>      int ret;
>  
> @@ -842,7 +854,7 @@
>      }
>  }
>  
> -static void accept_mutex_off(void)
> +static void accept_mutex_off_fcntl(void)
>  {
>      int ret;
>  
> @@ -857,12 +869,13 @@
>  	clean_child_exit(APEXIT_CHILDFATAL);
>      }
>  }
> +#endif
>  
> -#elif defined(USE_FLOCK_SERIALIZED_ACCEPT)
> +#if defined(USE_FLOCK_SERIALIZED_ACCEPT)
>  
> -static int lock_fd = -1;
> +static int flock_fd = -1;
>  
> -static void accept_mutex_cleanup(void *foo)
> +static void accept_mutex_cleanup_flock(void *foo)
>  {
>      unlink(ap_lock_fname);
>  }
> @@ -871,11 +884,11 @@
>   * Initialize mutex lock.
>   * Done by each child at it's birth
>   */
> -static void accept_mutex_child_init(pool *p)
> +static void accept_mutex_child_init_flock(pool *p)
>  {
>  
> -    lock_fd = ap_popenf(p, ap_lock_fname, O_WRONLY, 0600);
> -    if (lock_fd == -1) {
> +    flock_fd = ap_popenf(p, ap_lock_fname, O_WRONLY, 0600);
> +    if (flock_fd == -1) {
>  	ap_log_error(APLOG_MARK, APLOG_EMERG, server_conf,
>  		    "Child cannot open lock file: %s", ap_lock_fname);
>  	clean_child_exit(APEXIT_CHILDINIT);
> @@ -886,24 +899,24 @@
>   * Initialize mutex lock.
>   * Must be safe to call this on a restart.
>   */
> -static void accept_mutex_init(pool *p)
> +static void accept_mutex_init_flock(pool *p)
>  {
>      expand_lock_fname(p);
>      unlink(ap_lock_fname);
> -    lock_fd = ap_popenf(p, ap_lock_fname, O_CREAT | O_WRONLY | O_EXCL, 0600);
> -    if (lock_fd == -1) {
> +    flock_fd = ap_popenf(p, ap_lock_fname, O_CREAT | O_WRONLY | O_EXCL, 0600);
> +    if (flock_fd == -1) {
>  	ap_log_error(APLOG_MARK, APLOG_EMERG, server_conf,
>  		    "Parent cannot open lock file: %s", ap_lock_fname);
>  	exit(APEXIT_INIT);
>      }
> -    ap_register_cleanup(p, NULL, accept_mutex_cleanup, ap_null_cleanup);
> +    ap_register_cleanup(p, NULL, accept_mutex_cleanup_flock, ap_null_cleanup);
>  }
>  
> -static void accept_mutex_on(void)
> +static void accept_mutex_on_flock(void)
>  {
>      int ret;
>  
> -    while ((ret = flock(lock_fd, LOCK_EX)) < 0 && errno == EINTR)
> +    while ((ret = flock(flock_fd, LOCK_EX)) < 0 && errno == EINTR)
>  	continue;
>  
>      if (ret < 0) {
> @@ -913,20 +926,21 @@
>      }
>  }
>  
> -static void accept_mutex_off(void)
> +static void accept_mutex_off_flock(void)
>  {
> -    if (flock(lock_fd, LOCK_UN) < 0) {
> +    if (flock(flock_fd, LOCK_UN) < 0) {
>  	ap_log_error(APLOG_MARK, APLOG_EMERG, server_conf,
>  		    "flock: LOCK_UN: Error freeing accept lock. Exiting!");
>  	clean_child_exit(APEXIT_CHILDFATAL);
>      }
>  }
> +#endif
>  
> -#elif defined(USE_OS2SEM_SERIALIZED_ACCEPT)
> +#if defined(USE_OS2SEM_SERIALIZED_ACCEPT)
>  
>  static HMTX lock_sem = -1;
>  
> -static void accept_mutex_cleanup(void *foo)
> +static void accept_mutex_cleanup_os2sem(void *foo)
>  {
>      DosReleaseMutexSem(lock_sem);
>      DosCloseMutexSem(lock_sem);
> @@ -936,7 +950,7 @@
>   * Initialize mutex lock.
>   * Done by each child at it's birth
>   */
> -static void accept_mutex_child_init(pool *p)
> +static void accept_mutex_child_init_os2sem(pool *p)
>  {
>      int rc = DosOpenMutexSem(NULL, &lock_sem);
>  
> @@ -945,7 +959,7 @@
>  		    "Child cannot open lock semaphore, rc=%d", rc);
>  	clean_child_exit(APEXIT_CHILDINIT);
>      } else {
> -        ap_register_cleanup(p, NULL, accept_mutex_cleanup, ap_null_cleanup);
> +        ap_register_cleanup(p, NULL, accept_mutex_cleanup_os2sem, ap_null_cleanup);
>      }
>  }
>  
> @@ -953,7 +967,7 @@
>   * Initialize mutex lock.
>   * Must be safe to call this on a restart.
>   */
> -static void accept_mutex_init(pool *p)
> +static void accept_mutex_init_os2sem(pool *p)
>  {
>      int rc = DosCreateMutexSem(NULL, &lock_sem, DC_SEM_SHARED, FALSE);
>  
> @@ -963,10 +977,10 @@
>  	exit(APEXIT_INIT);
>      }
>  
> -    ap_register_cleanup(p, NULL, accept_mutex_cleanup, ap_null_cleanup);
> +    ap_register_cleanup(p, NULL, accept_mutex_cleanup_os2sem, ap_null_cleanup);
>  }
>  
> -static void accept_mutex_on(void)
> +static void accept_mutex_on_os2sem(void)
>  {
>      int rc = DosRequestMutexSem(lock_sem, SEM_INDEFINITE_WAIT);
>  
> @@ -977,7 +991,7 @@
>      }
>  }
>  
> -static void accept_mutex_off(void)
> +static void accept_mutex_off_os2sem(void)
>  {
>      int rc = DosReleaseMutexSem(lock_sem);
>      
> @@ -987,65 +1001,204 @@
>  	clean_child_exit(APEXIT_CHILDFATAL);
>      }
>  }
> +#endif
>  
> -#elif defined(USE_TPF_CORE_SERIALIZED_ACCEPT)
> +#if defined(USE_TPF_CORE_SERIALIZED_ACCEPT)
>  
>  static int tpf_core_held;
>  
> -static void accept_mutex_cleanup(void *foo)
> +static void accept_mutex_cleanup_tpfcore(void *foo)
>  {
>      if(tpf_core_held)
>          coruc(RESOURCE_KEY);
>  }
>  
> -#define accept_mutex_init(x)
> +#define accept_mutex_init_tpfcore(x)
>  
> -static void accept_mutex_child_init(pool *p)
> +static void accept_mutex_child_init_tpfcore(pool *p)
>  {
> -    ap_register_cleanup(p, NULL, accept_mutex_cleanup, ap_null_cleanup);
> +    ap_register_cleanup(p, NULL, accept_mutex_cleanup_tpfcore, ap_null_cleanup);
>      tpf_core_held = 0;
>  }
>  
> -static void accept_mutex_on(void)
> +static void accept_mutex_on_tpfcore(void)
>  {
>      corhc(RESOURCE_KEY);
>      tpf_core_held = 1;
>      ap_check_signals();
>  }
>  
> -static void accept_mutex_off(void)
> +static void accept_mutex_off_tpfcore(void)
>  {
>      coruc(RESOURCE_KEY);
>      tpf_core_held = 0;
>      ap_check_signals();
>  }
> +#endif
>  
> -#else
> -/* Default --- no serialization.  Other methods *could* go here,
> - * as #elifs...
> - */
>  #if !defined(MULTITHREAD)
>  /* Multithreaded systems don't complete between processes for
>   * the sockets. */
>  #define NO_SERIALIZED_ACCEPT
> -#define accept_mutex_child_init(x)
> -#define accept_mutex_init(x)
> -#define accept_mutex_on()
> -#define accept_mutex_off()
> +#define USE_NONE_CORE_SERIALIZED_ACCEPT
> +/* The below are actually not-needed, but are here for completeness.
> + * We take care of this in USE_NONE_CORE_SERIALIZED_ACCEPT */
> +#define accept_mutex_child_init_none(x)
> +#define accept_mutex_init_none(x)
> +#define accept_mutex_on_none()
> +#define accept_mutex_off_none()
> +#endif
> +
> +void (*accept_mutex_child_init_fptr)(pool *);
> +void (*accept_mutex_init_fptr)(pool *);
> +void (*accept_mutex_off_fptr)(void);
> +void (*accept_mutex_on_fptr)(void);
> +
> +#define AP_FPTR1(x,y)	{ if (x) ((* x)(y)); }
> +#define AP_FPTR0(x)	{ if (x) ((* x)()); }
> +
> +#define accept_mutex_child_init(x) 	AP_FPTR1(accept_mutex_child_init_fptr,x)
> +#define accept_mutex_init(x) 		AP_FPTR1(accept_mutex_init_fptr,x)
> +#define accept_mutex_off() 		AP_FPTR0(accept_mutex_off_fptr)
> +#define accept_mutex_on() 		AP_FPTR0(accept_mutex_on_fptr)
> +
> +char *default_mutex_method(void)
> +{
> +	char *t;
> +#if defined DEFAULT_SERIALIZED_ACCEPT_METHOD
> +	t = "DEFAULT_SERIALIZED_ACCEPT_METHOD";

Don't you want something like #DEFAULT_SERIALIZED_ACCEPT_METHOD (or
maybe with "##" instead of "#" to convert the value into a string?

> +#else
> +	t = "default";
>  #endif
> +#if defined USE_USLOCK_SERIALIZED_ACCEPT
> +	if ((!(strcasecmp(t,"default"))) || (!(strcasecmp(t,"uslock")))) {
> +		return "uslock";
> +	} else 

I'm confused.  How do we know that if t is "default" we should use
uslock?  Is it the order of the checks?  I guess it is a code error to
define more than one USE_foo_SERIALIZED_ACCEPT for the platform but
not define which one is the default?  Should we catch that here rather
than selecting the first match?

> +		return "Request serialized accept method not available";

"Requested", not "Request"

> +		return "Request serialized accept method not available";

same as previous comment, but I'd think one of these should never
fail... how about ap_assert() in one of them instead of returning an
error that we shouldn't have logic to check for?

-- 
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
       http://www.geocities.com/SiliconValley/Park/9289/
             Born in Roswell... married an alien...

Mime
View raw message