Subject: Re: [PATCH 04/10] -mm  clocksource: add some new API calls
From: john stultz <johnstul@us.ibm.com>
To: dwalker@mvista.com
Cc: akpm@osdl.org, linux-kernel@vger.kernel.org
In-Reply-To: <20060804032521.464282000@mvista.com>
References: <20060804032414.304636000@mvista.com>
	 <20060804032521.464282000@mvista.com>
Content-Type: text/plain
Date: Fri, 04 Aug 2006 12:06:40 -0700
Message-Id: <1154718400.5327.23.camel@localhost.localdomain>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 12616
Lines: 389

On Thu, 2006-08-03 at 20:24 -0700, dwalker@mvista.com wrote:
> plain text document attachment (clocksource_user_api.patch)
> This introduces some new API calls,
> 
> - clocksource_get_clock()
> 	Allows a clock lookup by name.
> - clocksource_rating_change()
> 	Used by a clocksource to signal a rating change. Replaces 
> 	reselect_clocksource()
> 
> I also moved the the clock source list to a plist, which removes some lookup
> overhead in the average case.
> 
> Signed-Off-By: Daniel Walker <dwalker@mvista.com>
> 
> ---
>  arch/i386/kernel/tsc.c      |    2 
>  include/linux/clocksource.h |   45 ++++++++++++-
>  kernel/time/clocksource.c   |  149 ++++++++++++++++++++++++++++----------------
>  3 files changed, 139 insertions(+), 57 deletions(-)
> 
> Index: linux-2.6.17/arch/i386/kernel/tsc.c
> ===================================================================
> --- linux-2.6.17.orig/arch/i386/kernel/tsc.c
> +++ linux-2.6.17/arch/i386/kernel/tsc.c
> @@ -351,7 +351,7 @@ static int tsc_update_callback(void)
>  	/* check to see if we should switch to the safe clocksource: */
>  	if (clocksource_tsc.rating != 50 && check_tsc_unstable()) {
>  		clocksource_tsc.rating = 50;
> -		clocksource_reselect();
> +		clocksource_rating_change(&clocksource_tsc);
>  		change = 1;
>  	}
>  
> Index: linux-2.6.17/include/linux/clocksource.h
> ===================================================================
> --- linux-2.6.17.orig/include/linux/clocksource.h
> +++ linux-2.6.17/include/linux/clocksource.h
> @@ -12,12 +12,20 @@
>  #include <linux/timex.h>
>  #include <linux/time.h>
>  #include <linux/list.h>
> +#include <linux/plist.h>
> +#include <linux/sysdev.h>
>  #include <asm/div64.h>
>  #include <asm/io.h>
>  
>  /* clocksource cycle base type */
>  typedef u64 cycle_t;
>  
> +/*
> + * This is the only generic clock, it should be used
> + * for early initialization.
> + */
> +extern struct clocksource clocksource_jiffies;
> +
>  /**
>   * struct clocksource - hardware abstraction for a free running counter
>   *	Provides mostly state-free accessors to the underlying hardware.
> @@ -51,7 +59,7 @@ typedef u64 cycle_t;
>   */
>  struct clocksource {
>  	char *name;
> -	struct list_head list;
> +	struct plist_node list;
>  	int rating;
>  	cycle_t (*read)(void);
>  	cycle_t mask;
> @@ -148,6 +156,25 @@ static inline s64 cyc2ns(struct clocksou
>  }
>  
>  /**
> + * ns2cyc - converts nanoseconds to clocksource cycles
> + * @cs:		Pointer to clocksource
> + * @ns:		Nanoseconds
> + *
> + * Uses the clocksource to convert nanoseconds back to cycles.
> + *
> + * XXX - This could use some mult_lxl_ll() asm optimization
> + */
> +static inline cycle_t ns2cyc(struct clocksource *cs, s64 ns)
> +{
> +	u64 ret = ns;
> +
> +	ret <<= cs->shift;
> +	do_div(ret, cs->mult);
> +
> +	return ret;
> +}
> +
> +/**
>   * clocksource_calculate_interval - Calculates a clocksource interval struct
>   *
>   * @c:		Pointer to clocksource.
> @@ -178,8 +205,18 @@ static inline void clocksource_calculate
>  
> 
>  /* used to install a new clocksource */
> -int clocksource_register(struct clocksource*);
> -void clocksource_reselect(void);
> -struct clocksource* clocksource_get_next(void);
> +extern int clocksource_register(struct clocksource*);
> +extern void clocksource_rating_change(struct clocksource*);
> +extern struct clocksource * clocksource_get_clock(char*);
>  
> +/**
> + * clocksource_get_best_clock - Finds highest rated clocksource
> + *
> + * Returns the highest rated clocksource. If none are register the
> + * jiffies clock is returned.
> + */
> +static inline struct clocksource * clocksource_get_best_clock(void)
> +{
> +	return clocksource_get_clock(NULL);
> +}
>  #endif /* _LINUX_CLOCKSOURCE_H */
> Index: linux-2.6.17/kernel/time/clocksource.c
> ===================================================================
> --- linux-2.6.17.orig/kernel/time/clocksource.c
> +++ linux-2.6.17/kernel/time/clocksource.c
> @@ -32,13 +32,18 @@
>  /* XXX - Would like a better way for initializing curr_clocksource */
>  extern struct clocksource clocksource_jiffies;
>  
> +/*
> + * Internally used to invert the rating, so lower is better.
> + */
> +#define CLOCKSOURCE_RATING(x)	(INT_MAX-x)

Since this is used for the plist bits, could it get a more explicit
name?


>  /*[Clocksource internal variables]---------
>   * curr_clocksource:
>   *	currently selected clocksource. Initialized to clocksource_jiffies.
>   * next_clocksource:
>   *	pending next selected clocksource.
>   * clocksource_list:
> - *	linked list with the registered clocksources
> + *	priority list with the registered clocksources
>   * clocksource_lock:
>   *	protects manipulations to curr_clocksource and next_clocksource
>   *	and the clocksource_list
> @@ -47,7 +52,8 @@ extern struct clocksource clocksource_ji
>   */
>  static struct clocksource *curr_clocksource = &clocksource_jiffies;
>  static struct clocksource *next_clocksource;
> -static LIST_HEAD(clocksource_list);
> +static struct plist_head clocksource_list =
> +	PLIST_HEAD_INIT(clocksource_list, clocksource_lock);
>  static DEFINE_SPINLOCK(clocksource_lock);
>  static char override_name[32];
>  
> @@ -70,84 +76,111 @@ struct clocksource *clocksource_get_next
>  }
>  
>  /**
> - * select_clocksource - Finds the best registered clocksource.
> + * __is_registered - Returns a clocksource if it's registered
> + * @name:		name of the clocksource to return
>   *
>   * Private function. Must hold clocksource_lock when called.
>   *
> - * Looks through the list of registered clocksources, returning
> - * the one with the highest rating value. If there is a clocksource
> - * name that matches the override string, it returns that clocksource.
> + * Returns the clocksource if registered, zero otherwise.
> + * If no clocksources are registered the jiffies clock is
> + * returned.
>   */
> -static struct clocksource *select_clocksource(void)
> +static struct clocksource * __is_registered(char * name)
>  {
> -	struct clocksource *best = NULL;
> -	struct list_head *tmp;
> +	struct plist_node *tmp;
>  
> -	list_for_each(tmp, &clocksource_list) {
> +	plist_for_each(tmp, &clocksource_list) {
>  		struct clocksource *src;
>  
>  		src = list_entry(tmp, struct clocksource, list);
> -		if (!best)
> -			best = src;
> -
> -		/* check for override: */
> -		if (strlen(src->name) == strlen(override_name) &&
> -		    !strcmp(src->name, override_name)) {
> -			best = src;
> -			break;
> -		}
> -		/* pick the highest rating: */
> -		if (src->rating > best->rating)
> -		 	best = src;
> +		if (!strcmp(src->name, name))
> +			return src;
>  	}
>  
> -	return best;
> +	return 0;
>  }
>  
>  /**
> - * is_registered_source - Checks if clocksource is registered
> - * @c:		pointer to a clocksource
> + * __get_clock - Finds a specific clocksource
> + * @name:		name of the clocksource to return
>   *
> - * Private helper function. Must hold clocksource_lock when called.
> + * Private function. Must hold clocksource_lock when called.
>   *
> - * Returns one if the clocksource is already registered, zero otherwise.
> + * Returns the clocksource if registered, zero otherwise.
> + * If the @name is null the highest rated clock is returned.
>   */
> -static int is_registered_source(struct clocksource *c)
> +static inline struct clocksource * __get_clock(char * name)
>  {
> -	int len = strlen(c->name);
> -	struct list_head *tmp;
>  
> -	list_for_each(tmp, &clocksource_list) {
> -		struct clocksource *src;
> +	if (unlikely(plist_head_empty(&clocksource_list)))
> +		return &clocksource_jiffies;
>  
> -		src = list_entry(tmp, struct clocksource, list);
> -		if (strlen(src->name) == len &&	!strcmp(src->name, c->name))
> -			return 1;
> -	}
> +	if (!name)
> +		return plist_first_entry(&clocksource_list, struct clocksource,
> +					 list);
>  
> -	return 0;
> +	return __is_registered(name);
> +}
> +
> +/**
> + * clocksource_get_clock - Finds a specific clocksource
> + * @name:		name of the clocksource to return
> + *
> + * Returns the clocksource if registered, zero otherwise.
> + */
> +struct clocksource * clocksource_get_clock(char * name)
> +{
> +	struct clocksource * ret;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&clocksource_lock, flags);
> +	ret = __get_clock(name);
> +	spin_unlock_irqrestore(&clocksource_lock, flags);
> +	return ret;
> +}
> +
> +
> +/**
> + * select_clocksource - Finds the best registered clocksource.
> + *
> + * Private function. Must hold clocksource_lock when called.
> + *
> + * Looks through the list of registered clocksources, returning
> + * the one with the highest rating value. If there is a clocksource
> + * name that matches the override string, it returns that clocksource.
> + */
> +static struct clocksource *select_clocksource(void)
> +{
> +	if (!*override_name)
> +		return plist_first_entry(&clocksource_list, struct clocksource,
> +					 list);
> +	return get_clock(override_name);
>  }

This all looks good.


>  /**
>   * clocksource_register - Used to install new clocksources
>   * @t:		clocksource to be registered
>   *
> - * Returns -EBUSY if registration fails, zero otherwise.
> + * Returns -EBUSY clock is already registered,
> + * Returns -EINVAL if clocksource is invalid,
> + * Return zero otherwise.
>   */
>  int clocksource_register(struct clocksource *c)
>  {
>  	int ret = 0;
>  	unsigned long flags;
>  
> +	if (unlikely(!c))
> +		return -EINVAL;
> +

I'm not sure I understand the need for this. Is it really likely someone
would pass NULL to clocksource_register()?


>  	spin_lock_irqsave(&clocksource_lock, flags);
> -	/* check if clocksource is already registered */
> -	if (is_registered_source(c)) {
> -		printk("register_clocksource: Cannot register %s. "
> +	if (unlikely(!plist_node_empty(&c->list) && __is_registered(c->name))) {
> +		printk("register_clocksource: Cannot register %s clocksource. "
>  		       "Already registered!", c->name);
>  		ret = -EBUSY;
>  	} else {
> -		/* register it */
> - 		list_add(&c->list, &clocksource_list);
> +		plist_node_init(&c->list, CLOCKSOURCE_RATING(c->rating));
> + 		plist_add(&c->list, &clocksource_list);
>  		/* scan the registered clocksources, and pick the best one */
>  		next_clocksource = select_clocksource();
>  	}
> @@ -157,21 +190,32 @@ int clocksource_register(struct clocksou
>  EXPORT_SYMBOL(clocksource_register);
>  
>  /**
> - * clocksource_reselect - Rescan list for next clocksource
> + * clocksource_rating_change - Allows dynamic rating changes for register
> + *                           clocksources.
>   *
> - * A quick helper function to be used if a clocksource changes its
> - * rating. Forces the clocksource list to be re-scanned for the best
> - * clocksource.
> + * Signals that a clocksource is dynamically changing it's rating.
> + * This could happen if a clocksource becomes more/less stable.
>   */
> -void clocksource_reselect(void)
> +void clocksource_rating_change(struct clocksource *c)
>  {
>  	unsigned long flags;
>  
> +	if (unlikely(plist_node_empty(&c->list)))
> +		return;
> +
>  	spin_lock_irqsave(&clocksource_lock, flags);
> +
> +	/*
> +	 * Re-register the clocksource with it's new rating.
> +	 */
> +	plist_del(&c->list, &clocksource_list);
> +	plist_node_init(&c->list, CLOCKSOURCE_RATING(c->rating));
> +	plist_add(&c->list, &clocksource_list);
> +
>  	next_clocksource = select_clocksource();
>  	spin_unlock_irqrestore(&clocksource_lock, flags);
>  }
> -EXPORT_SYMBOL(clocksource_reselect);
> +EXPORT_SYMBOL(clocksource_rating_change);
>  
>  /**
>   * sysfs_show_current_clocksources - sysfs interface for current clocksource
> @@ -236,16 +280,17 @@ static ssize_t sysfs_override_clocksourc
>   * @dev:	unused
>   * @buf:	char buffer to be filled with clocksource list
>   *
> - * Provides sysfs interface for listing registered clocksources
> + * Provides sysfs interface for listing registered clocksources.
> + * Output in priority sorted order.
>   */
>  static ssize_t
>  sysfs_show_available_clocksources(struct sys_device *dev, char *buf)
>  {
> -	struct list_head *tmp;
> +	struct plist_node *tmp;
>  	char *curr = buf;
>  
>  	spin_lock_irq(&clocksource_lock);
> -	list_for_each(tmp, &clocksource_list) {
> +	plist_for_each(tmp, &clocksource_list) {
>  		struct clocksource *src;
>  
>  		src = list_entry(tmp, struct clocksource, list);
> 

No real objections to this one.

thanks
-john


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/