Add statistic to rtems_disk_device and bdbuf

[Sebastian Huber]

Hi Xiang,

thanks for your first proposal.  I would call the statistics

rtems_bdbuf_stats
rtems_disk_stats

to keep the name a bit shorter.

On 05/23/2012 12:50 PM, Xiang Cui wrote:
> Hi
>
> I want to add new struct to the rtems_disk_device and bdbuf. All of
> them are counters for read/write and cache hit number. This is a very
> basic ideal now. These change will help to measure the IO performance
> and uncover the bdbuf state.
>
> All these statistics will be updated in bdbuf, I think.
>
> The changes in bdbuf.h and diskdevs.h may like this.
>
> diff --git a/cpukit/libblock/include/rtems/bdbuf.h
> b/cpukit/libblock/include/rtems/bdbuf.h
> index 134a0ce..25ad374 100644
> --- a/cpukit/libblock/include/rtems/bdbuf.h
> +++ b/cpukit/libblock/include/rtems/bdbuf.h
> @@ -292,6 +292,18 @@ typedef enum
>   } rtems_bdbuf_buf_state;
>
>   /**
> +* @brief Statistic for bdbuf.
> +*/
> +typedef struct {
> +  unsigned swapout_execute_count;   /**<  counter for swapout execute */
> +  unsigned recycle_count;           /**<  counter for the recycle */
> +  unsigned total_write_count;       /**<  counter for the total write action */
> +  unsigned total_read_count;        /**<  counter for the total read action */
> +  unsigned total_write_hit;         /**<  counter for the total write hit  */

How can a write miss occur?

> +  unsigned total_read_hit;          /**<  counter for the total read hit */
> +} rtems_bdbuf_statistic;

Please use uint32_t or similar instead of unsigned to get at least 32-bits.

> +
> +/**
>    * Forward reference to the block.
>    */
>   struct rtems_bdbuf_group;
> @@ -644,6 +656,14 @@ rtems_bdbuf_purge_dev (const rtems_disk_device *dd);
>   rtems_status_code
>   rtems_bdbuf_set_block_size (rtems_disk_device *dd, uint32_t block_size);
>
> +
> +/**
> +* @brief Return the statistic for bdbuf.
> +*
> +* @return pointer to rtems_bdbuf_statistic.
> +*/
> +const rtems_bdbuf_statistic *rtems_bdbuf_get_statistic(void);

This is problematic since the statistics change with time.  Better is to return 
a snapshot

void rtems_bdbuf_get_statistic(rtems_bdbuf_stats *stats);

with a proper cache lock/unlock inside.

> +
>   /** @} */
>
>   #ifdef __cplusplus
> diff --git a/cpukit/libblock/include/rtems/diskdevs.h
> b/cpukit/libblock/include/rtems/diskdevs.h
> index 2e3f753..e969894 100644
> --- a/cpukit/libblock/include/rtems/diskdevs.h
> +++ b/cpukit/libblock/include/rtems/diskdevs.h
> @@ -55,6 +55,30 @@ typedef int (*rtems_block_device_ioctl)(
>   );
>
>   /**
> + * @brief Statistic to count the write and read number for bdbuf.
> + *
> + * These statistics counted by block size.
> + */
> +typedef struct {
> +  /**
> +  * @brief counter for write number since the disk device was created
> +  */
> +  unsigned write_count;
> +  /**
> +  * @brief counter for read number since the disk device was created
> +  */
> +  unsigned read_count;
> +  /**
> +  * @brief counter for write hit in bdbuf
> +  */
> +  unsigned write_hit;
> +  /**
> +  * @brief counter for read hit in bdbuf
> +  */
> +  unsigned read_hit;
> +} rtems_disk_device_statistic;
> +
> +/**
>    * @brief Description of a disk device (logical and physical disks).
>    *
>    * An array of pointer tables to rtems_disk_device structures is maintained.
> @@ -153,6 +177,13 @@ struct rtems_disk_device {
>      * releases this disk.
>      */
>     bool deleted;
> +
> + /**
> +  * @brief statistic for disk device. The logical disks have the same
> +  * pointer to these physical disk.
> +  */
> +  rtems_disk_device_statistic *statistic;

Why should physical and logical disk share the statistics?  The physical and 
logical disks are from the cache point of view separate entities.

[...]

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.