[lustre-devel] [PATCH 1/8] staging: lustre: lnet: simplify lnet_eq_wait_locked

NeilBrown neilb at suse.com
Sun Jun 24 18:42:44 PDT 2018


On Sun, Jun 24 2018, James Simmons wrote:

> We can simplify the code by taking advantage of the behavior
> of schedule_timeout_interruptible(). Instead of testing if
> tms is less than zero we can pass in a signed long that
> schedule_timeout_interruptible is expecting and for the case
> of no timeout we can pass in MAX_SCHEDULE_TIMEOUT.
>
> Signed-off-by: James Simmons <uja.ornl at yahoo.com>
> WC-bug-id: https://jira.whamcloud.com/browse/LU-9019
> Reviewed-on: https://review.whamcloud.com/23147
> Reviewed-by: Doug Oucharek <dougso at me.com>
> Reviewed-by: Dmitry Eremin <dmitry.eremin at intel.com>
> Reviewed-by: Oleg Drokin <green at whamcloud.com>
> Signed-off-by: James Simmons <jsimmons at infradead.org>
> ---
>  drivers/staging/lustre/include/linux/lnet/api.h    |  2 +-
>  .../lustre/include/uapi/linux/lnet/lnet-types.h    |  2 --
>  drivers/staging/lustre/lnet/lnet/api-ni.c          | 38 ++++++++++++++--------
>  drivers/staging/lustre/lnet/lnet/lib-eq.c          | 26 +++++----------
>  4 files changed, 33 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/staging/lustre/include/linux/lnet/api.h b/drivers/staging/lustre/include/linux/lnet/api.h
> index dae2e4f..2242921 100644
> --- a/drivers/staging/lustre/include/linux/lnet/api.h
> +++ b/drivers/staging/lustre/include/linux/lnet/api.h
> @@ -168,7 +168,7 @@ int LNetEQAlloc(unsigned int       count_in,
>  
>  int LNetEQPoll(struct lnet_handle_eq *eventqs_in,
>  	       int		 neq_in,
> -	       int		 timeout_ms,
> +	       signed long	 timeout,
>  	       int		 interruptible,
>  	       struct lnet_event *event_out,
>  	       int		*which_eq_out);
> diff --git a/drivers/staging/lustre/include/uapi/linux/lnet/lnet-types.h b/drivers/staging/lustre/include/uapi/linux/lnet/lnet-types.h
> index 1be9b7a..14715a8 100644
> --- a/drivers/staging/lustre/include/uapi/linux/lnet/lnet-types.h
> +++ b/drivers/staging/lustre/include/uapi/linux/lnet/lnet-types.h
> @@ -77,8 +77,6 @@
>  #define LNET_PID_USERFLAG 0x80000000 /* set in userspace peers */
>  #define LNET_PID_LUSTRE	  12345
>  
> -#define LNET_TIME_FOREVER (-1)
> -
>  /* how an LNET NID encodes net:address */
>  /** extract the address part of an lnet_nid_t */
>  
> diff --git a/drivers/staging/lustre/lnet/lnet/api-ni.c b/drivers/staging/lustre/lnet/lnet/api-ni.c
> index f9ed697..878c92c 100644
> --- a/drivers/staging/lustre/lnet/lnet/api-ni.c
> +++ b/drivers/staging/lustre/lnet/lnet/api-ni.c
> @@ -59,7 +59,7 @@
>  module_param(rnet_htable_size, int, 0444);
>  MODULE_PARM_DESC(rnet_htable_size, "size of remote network hash table");
>  
> -static int lnet_ping(struct lnet_process_id id, int timeout_ms,
> +static int lnet_ping(struct lnet_process_id id, signed long timeout,
>  		     struct lnet_process_id __user *ids, int n_ids);
>  
>  static char *
> @@ -2052,17 +2052,29 @@ void lnet_lib_exit(void)
>  	case IOC_LIBCFS_LNET_FAULT:
>  		return lnet_fault_ctl(data->ioc_flags, data);
>  
> -	case IOC_LIBCFS_PING:
> +	case IOC_LIBCFS_PING: {
> +		signed long timeout;
> +
>  		id.nid = data->ioc_nid;
>  		id.pid = data->ioc_u32[0];
> -		rc = lnet_ping(id, data->ioc_u32[1], /* timeout */
> -			       data->ioc_pbuf1,
> +
> +		/* Don't block longer than 2 minutes */
> +		if (data->ioc_u32[1] > 120 * MSEC_PER_SEC)
> +			return -EINVAL;
> +
> +		/* If timestamp is negative then disable timeout */
> +		if ((s32)data->ioc_u32[1] < 0)
> +			timeout = MAX_SCHEDULE_TIMEOUT;
> +		else
> +			timeout = msecs_to_jiffies(data->ioc_u32[1]);
> +
> +		rc = lnet_ping(id, timeout, data->ioc_pbuf1,
>  			       data->ioc_plen1 / sizeof(struct lnet_process_id));
>  		if (rc < 0)
>  			return rc;
>  		data->ioc_count = rc;
>  		return 0;
> -
> +	}
>  	default:
>  		ni = lnet_net2ni(data->ioc_net);
>  		if (!ni)
> @@ -2126,7 +2138,7 @@ void LNetDebugPeer(struct lnet_process_id id)
>  }
>  EXPORT_SYMBOL(LNetGetId);
>  
> -static int lnet_ping(struct lnet_process_id id, int timeout_ms,
> +static int lnet_ping(struct lnet_process_id id, signed long timeout,
>  		     struct lnet_process_id __user *ids, int n_ids)
>  {
>  	struct lnet_handle_eq eqh;
> @@ -2136,7 +2148,7 @@ static int lnet_ping(struct lnet_process_id id, int timeout_ms,
>  	int which;
>  	int unlinked = 0;
>  	int replied = 0;
> -	const int a_long_time = 60000; /* mS */
> +	const signed long a_long_time = msecs_to_jiffies(60 * MSEC_PER_SEC);

I've changed this to
	a_long_toime = 60*HZ;

as that is the more common pattern in the kernel.

>  	int infosz;
>  	struct lnet_ping_info *info;
>  	struct lnet_process_id tmpid;
> @@ -2147,10 +2159,8 @@ static int lnet_ping(struct lnet_process_id id, int timeout_ms,
>  
>  	infosz = offsetof(struct lnet_ping_info, pi_ni[n_ids]);
>  
> -	if (n_ids <= 0 ||
> -	    id.nid == LNET_NID_ANY ||
> -	    timeout_ms > 500000 ||	      /* arbitrary limit! */
> -	    n_ids > 20)			 /* arbitrary limit! */
> +	/* n_ids limit is arbitrary */
> +	if (n_ids <= 0 || n_ids > 20 || id.nid == LNET_NID_ANY)
>  		return -EINVAL;
>  
>  	if (id.pid == LNET_PID_ANY)
> @@ -2194,13 +2204,13 @@ static int lnet_ping(struct lnet_process_id id, int timeout_ms,
>  
>  		/* NB must wait for the UNLINK event below... */
>  		unlinked = 1;
> -		timeout_ms = a_long_time;
> +		timeout = a_long_time;
>  	}
>  
>  	do {
>  		/* MUST block for unlink to complete */
>  
> -		rc2 = LNetEQPoll(&eqh, 1, timeout_ms, !unlinked,
> +		rc2 = LNetEQPoll(&eqh, 1, timeout, !unlinked,
>  				 &event, &which);
>  
>  		CDEBUG(D_NET, "poll %d(%d %d)%s\n", rc2,
> @@ -2222,7 +2232,7 @@ static int lnet_ping(struct lnet_process_id id, int timeout_ms,
>  				LNetMDUnlink(mdh);
>  				/* No assertion (racing with network) */
>  				unlinked = 1;
> -				timeout_ms = a_long_time;
> +				timeout = a_long_time;
>  			} else if (!rc2) {
>  				/* timed out waiting for unlink */
>  				CWARN("ping %s: late network completion\n",
> diff --git a/drivers/staging/lustre/lnet/lnet/lib-eq.c b/drivers/staging/lustre/lnet/lnet/lib-eq.c
> index c78e703..366ad8a 100644
> --- a/drivers/staging/lustre/lnet/lnet/lib-eq.c
> +++ b/drivers/staging/lustre/lnet/lnet/lib-eq.c
> @@ -308,13 +308,12 @@
>   */
>  
>  static int
> -lnet_eq_wait_locked(int *timeout_ms, long state)
> +lnet_eq_wait_locked(signed long *timeout, long state)
>  __must_hold(&the_lnet.ln_eq_wait_lock)
>  {
> -	int tms = *timeout_ms;
> +	signed long tms = *timeout;
>  	int wait;
>  	wait_queue_entry_t wl;
> -	unsigned long now;
>  
>  	if (!tms)
>  		return -ENXIO; /* don't want to wait and no new event */
> @@ -325,18 +324,9 @@
>  
>  	lnet_eq_wait_unlock();
>  
> -	if (tms < 0) {
> -		schedule();
> -	} else {
> -		now = jiffies;
> -		schedule_timeout(msecs_to_jiffies(tms));
> -		tms -= jiffies_to_msecs(jiffies - now);
> -		if (tms < 0) /* no more wait but may have new event */
> -			tms = 0;
> -	}
> -
> +	tms = schedule_timeout_interruptible(tms);

I've changed this to schedule_timeout().
The task state has already been set (to INTERRUPTIBLE or NOLOAD) and
we don't want to set it again.

Thanks,
NeilBrown


>  	wait = tms; /* might need to call here again */
> -	*timeout_ms = tms;
> +	*timeout = tms;
>  
>  	lnet_eq_wait_lock();
>  	remove_wait_queue(&the_lnet.ln_eq_waitq, &wl);
> @@ -356,8 +346,8 @@
>   * fixed period, or block indefinitely.
>   *
>   * \param eventqs,neq An array of EQ handles, and size of the array.
> - * \param timeout_ms Time in milliseconds to wait for an event to occur on
> - * one of the EQs. The constant LNET_TIME_FOREVER can be used to indicate an
> + * \param timeout Time in jiffies to wait for an event to occur on
> + * one of the EQs. The constant MAX_SCHEDULE_TIMEOUT can be used to indicate an
>   * infinite timeout.
>   * \param interruptible, if true, use TASK_INTERRUPTIBLE, else TASK_NOLOAD
>   * \param event,which On successful return (1 or -EOVERFLOW), \a event will
> @@ -372,7 +362,7 @@
>   * \retval -ENOENT    If there's an invalid handle in \a eventqs.
>   */
>  int
> -LNetEQPoll(struct lnet_handle_eq *eventqs, int neq, int timeout_ms,
> +LNetEQPoll(struct lnet_handle_eq *eventqs, int neq, signed long timeout,
>  	   int interruptible,
>  	   struct lnet_event *event, int *which)
>  {
> @@ -414,7 +404,7 @@
>  		 *  0 : don't want to wait anymore, but might have new event
>  		 *      so need to call dequeue again
>  		 */
> -		wait = lnet_eq_wait_locked(&timeout_ms,
> +		wait = lnet_eq_wait_locked(&timeout,
>  					   interruptible ? TASK_INTERRUPTIBLE
>  					   : TASK_NOLOAD);
>  		if (wait < 0) /* no new event */
> -- 
> 1.8.3.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20180625/42a1c406/attachment.sig>


More information about the lustre-devel mailing list