[PATCH] libio: Remove special-case reference count

Gedare Bloom gedare at rtems.org
Thu Sep 7 22:08:48 UTC 2017


I understand the motivation. I need to look carefully at whether this
breaks the special case, and how to otherwise fix it.

On Wed, Sep 6, 2017 at 2:27 AM, Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
> The top-level IO library structures should contain no special-case data.
>
> Update #2859.
> ---
>  cpukit/libcsupport/include/rtems/libio.h    |  1 -
>  cpukit/libcsupport/include/rtems/libio_.h   | 41 -----------------------------
>  cpukit/libcsupport/src/libio.c              | 32 ++--------------------
>  cpukit/posix/include/rtems/posix/mmanimpl.h | 12 ++++-----
>  cpukit/posix/src/mmap.c                     | 24 ++++++++---------
>  cpukit/posix/src/munmap.c                   | 29 ++++++--------------
>  6 files changed, 27 insertions(+), 112 deletions(-)
>
> diff --git a/cpukit/libcsupport/include/rtems/libio.h b/cpukit/libcsupport/include/rtems/libio.h
> index 8226d18ba2..7022de671c 100644
> --- a/cpukit/libcsupport/include/rtems/libio.h
> +++ b/cpukit/libcsupport/include/rtems/libio.h
> @@ -1320,7 +1320,6 @@ struct rtems_libio_tt {
>    rtems_driver_name_t                    *driver;
>    off_t                                   offset;    /* current offset into file */
>    uint32_t                                flags;
> -  uint32_t                                mapping_refcnt; /* current mappings */
>    rtems_filesystem_location_info_t        pathinfo;
>    uint32_t                                data0;     /* private to "driver" */
>    void                                   *data1;     /* ... */
> diff --git a/cpukit/libcsupport/include/rtems/libio_.h b/cpukit/libcsupport/include/rtems/libio_.h
> index 695a4c45a5..c2fb975bf7 100644
> --- a/cpukit/libcsupport/include/rtems/libio_.h
> +++ b/cpukit/libcsupport/include/rtems/libio_.h
> @@ -304,47 +304,6 @@ void rtems_libio_free(
>    rtems_libio_t *iop
>  );
>
> -/**
> - * Garbage collects the free libio in case it was previously freed but there
> - * were still references to it.
> - */
> -void rtems_libio_check_deferred_free( rtems_libio_t *iop );
> -
> -/**
> - * Increment the reference count tracking number of mmap mappings of a file.
> - * Returns the updated reference count value.
> - */
> -static inline uint32_t rtems_libio_increment_mapping_refcnt(rtems_libio_t *iop)
> -{
> -  uint32_t refcnt;
> -  rtems_libio_lock();
> -  refcnt = ++iop->mapping_refcnt;
> -  rtems_libio_unlock();
> -  return refcnt;
> -}
> -
> -/**
> - * Decrement the reference count tracking number of mmap mappings of a file.
> - * Returns the updated reference count value.
> - */
> -static inline uint32_t rtems_libio_decrement_mapping_refcnt(rtems_libio_t *iop)
> -{
> -  uint32_t refcnt;
> -  rtems_libio_lock();
> -  refcnt = --iop->mapping_refcnt;
> -  rtems_libio_unlock();
> -  return refcnt;
> -}
> -
> -static inline bool rtems_libio_is_mapped(rtems_libio_t *iop)
> -{
> -  bool is_mapped;
> -  rtems_libio_lock();
> -  is_mapped = iop->mapping_refcnt != 0;
> -  rtems_libio_unlock();
> -  return is_mapped;
> -}
> -
>  /*
>   *  File System Routine Prototypes
>   */
> diff --git a/cpukit/libcsupport/src/libio.c b/cpukit/libcsupport/src/libio.c
> index e89634f090..22be6411a2 100644
> --- a/cpukit/libcsupport/src/libio.c
> +++ b/cpukit/libcsupport/src/libio.c
> @@ -138,36 +138,8 @@ void rtems_libio_free(
>    rtems_libio_lock();
>
>      iop->flags = 0;
> -    /* If the mapping_refcnt is non-zero, the deferred free will be
> -     * called by munmap. The iop is no longer good to use, but it cannot
> -     * be recycled until the mapped file is unmapped. deferred free knows
> -     * it can recycle the iop in case flags == 0 and iop->data1 == iop,
> -     * since these two conditions are not otherwise satisifed at
> -     * the same time. It may be possible that iop->data1 == iop when
> -     * flags != 0 because data1 is private to the driver. However, flags == 0
> -     * means a freed iop, and an iop on the freelist cannot store a pointer
> -     * to itself in data1, or else the freelist is corrupted. We can't use
> -     * NULL in data1 as an indicator because it is used by the tail of the
> -     * freelist. */
> -    if ( iop->mapping_refcnt == 0 ) {
> -      iop->data1 = rtems_libio_iop_freelist;
> -      rtems_libio_iop_freelist = iop;
> -    } else {
> -      iop->data1 = iop;
> -    }
> +    iop->data1 = rtems_libio_iop_freelist;
> +    rtems_libio_iop_freelist = iop;
>
>    rtems_libio_unlock();
>  }
> -
> -void rtems_libio_check_deferred_free(
> -  rtems_libio_t *iop
> -)
> -{
> -  rtems_libio_lock();
> -    if ( iop->mapping_refcnt == 0 && iop->flags == 0 && iop->data1 == iop) {
> -      /* No mappings and rtems_libio_free already called, recycle the iop */
> -      iop->data1 = rtems_libio_iop_freelist;
> -      rtems_libio_iop_freelist = iop;
> -    }
> -  rtems_libio_unlock();
> -}
> diff --git a/cpukit/posix/include/rtems/posix/mmanimpl.h b/cpukit/posix/include/rtems/posix/mmanimpl.h
> index ff59d911ca..e1cc672331 100644
> --- a/cpukit/posix/include/rtems/posix/mmanimpl.h
> +++ b/cpukit/posix/include/rtems/posix/mmanimpl.h
> @@ -18,6 +18,7 @@
>
>  #include <rtems/libio_.h>
>  #include <rtems/chain.h> /* FIXME: use score chains for proper layering? */
> +#include <rtems/posix/shm.h>
>
>  #ifdef __cplusplus
>  extern "C" {
> @@ -29,12 +30,11 @@ extern "C" {
>   * Every mmap'ed region has a mapping.
>   */
>  typedef struct mmap_mappings_s {
> -  rtems_chain_node node;  /**< The mapping chain's node */
> -  void*            addr;  /**< The address of the mapped memory */
> -  size_t           len;   /**< The length of memory mapped */
> -  int              flags; /**< The mapping flags */
> -  rtems_libio_t   *iop;   /**< The mapped object's file descriptor pointer */
> -  bool             is_shared_shm; /**< True if MAP_SHARED of shared memory */
> +  rtems_chain_node   node;  /**< The mapping chain's node */
> +  void*              addr;  /**< The address of the mapped memory */
> +  size_t             len;   /**< The length of memory mapped */
> +  int                flags; /**< The mapping flags */
> +  POSIX_Shm_Control *shm;   /**< The shared memory object or NULL */
>  } mmap_mapping;
>
>  extern rtems_chain_control mmap_mappings;
> diff --git a/cpukit/posix/src/mmap.c b/cpukit/posix/src/mmap.c
> index b5af180d3d..9d9c0634ff 100644
> --- a/cpukit/posix/src/mmap.c
> +++ b/cpukit/posix/src/mmap.c
> @@ -48,6 +48,7 @@ void *mmap(
>    bool            map_anonymous;
>    bool            map_shared;
>    bool            map_private;
> +  bool            is_shared_shm;
>    int             err;
>
>    map_fixed = (flags & MAP_FIXED) == MAP_FIXED;
> @@ -194,7 +195,6 @@ void *mmap(
>    memset( mapping, 0, sizeof( mmap_mapping ));
>    mapping->len = len;
>    mapping->flags = flags;
> -  mapping->iop = iop;
>
>    if ( !map_anonymous ) {
>      /*
> @@ -206,19 +206,19 @@ void *mmap(
>      if ( S_ISREG( sb.st_mode ) || S_ISBLK( sb.st_mode ) ||
>           S_ISCHR( sb.st_mode ) || S_ISFIFO( sb.st_mode ) ||
>           S_ISSOCK( sb.st_mode ) ) {
> -      mapping->is_shared_shm = false;
> +      is_shared_shm = false;
>      } else {
> -      mapping->is_shared_shm = true;
> +      is_shared_shm = true;
>      }
>    } else {
> -    mapping->is_shared_shm = false;
> +    is_shared_shm = false;
>    }
>
>    if ( map_fixed ) {
>      mapping->addr = addr;
>    } else if ( map_private ) {
>      /* private mappings of shared memory do not need special treatment. */
> -    mapping->is_shared_shm = false;
> +    is_shared_shm = false;
>      posix_memalign( &mapping->addr, PAGE_SIZE, len );
>      if ( !mapping->addr ) {
>        free( mapping );
> @@ -228,7 +228,7 @@ void *mmap(
>    }
>
>    /* MAP_FIXED is not supported for shared memory objects with MAP_SHARED. */
> -  if ( map_fixed && mapping->is_shared_shm ) {
> +  if ( map_fixed && is_shared_shm ) {
>      free( mapping );
>      errno = ENOTSUP;
>      return MAP_FAILED;
> @@ -280,6 +280,11 @@ void *mmap(
>        memset( mapping->addr, 0, len );
>      }
>    } else if ( map_shared ) {
> +    if ( is_shared_shm ) {
> +      /* FIXME: This use of implementation details is a hack. */
> +      mapping->shm = iop_to_shm( iop );
> +    }
> +
>      err = (*iop->pathinfo.handlers->mmap_h)(
>          iop, &mapping->addr, len, prot, off );
>      if ( err != 0 ) {
> @@ -289,13 +294,6 @@ void *mmap(
>      }
>    }
>
> -  if ( iop != NULL ) {
> -    /* add an extra reference to the file associated with fildes that
> -     * is not removed by a subsequent close().  This reference shall be removed
> -     * when there are no more mappings to the file. */
> -    rtems_libio_increment_mapping_refcnt(iop);
> -  }
> -
>    rtems_chain_append_unprotected( &mmap_mappings, &mapping->node );
>
>    mmap_mappings_lock_release( );
> diff --git a/cpukit/posix/src/munmap.c b/cpukit/posix/src/munmap.c
> index fb9bb872e0..5348be7a51 100644
> --- a/cpukit/posix/src/munmap.c
> +++ b/cpukit/posix/src/munmap.c
> @@ -16,23 +16,14 @@
>  #include <stdlib.h>
>  #include <sys/mman.h>
>
> -#include <rtems/posix/mmanimpl.h>
> +#include <rtems/posix/mmanimpl.h>
>  #include <rtems/posix/shmimpl.h>
>
> -static void shm_munmap( rtems_libio_t *iop )
> -{
> -  POSIX_Shm_Control *shm = iop_to_shm( iop );
> -
> -  /* decrement mmap's shm reference_count and maybe delete the object */
> -  POSIX_Shm_Attempt_delete(shm);
> -}
> -
>  int munmap(void *addr, size_t len)
>  {
> -  mmap_mapping      *mapping;
> -  rtems_chain_node  *node;
> -  uint32_t           refcnt;
> -
> +  mmap_mapping     *mapping;
> +  rtems_chain_node *node;
> +
>    /*
>     * Clear errno.
>     */
> @@ -60,17 +51,13 @@ int munmap(void *addr, size_t len)
>      if ( ( addr >= mapping->addr ) &&
>           ( addr < ( mapping->addr + mapping->len )) ) {
>        rtems_chain_extract_unprotected( node );
> +
>        /* FIXME: generally need a way to clean-up the backing object, but
>         * currently it only matters for MAP_SHARED shm objects. */
> -      if ( mapping->is_shared_shm == true ) {
> -        shm_munmap(mapping->iop);
> -      }
> -      if ( mapping->iop != NULL ) {
> -        refcnt = rtems_libio_decrement_mapping_refcnt(mapping->iop);
> -        if ( refcnt == 0 ) {
> -          rtems_libio_check_deferred_free(mapping->iop);
> -        }
> +      if ( mapping->shm != NULL ) {
> +        POSIX_Shm_Attempt_delete(mapping->shm);
>        }
> +
>        /* only free the mapping address for non-fixed mapping */
>        if (( mapping->flags & MAP_FIXED ) != MAP_FIXED ) {
>          /* only free the mapping address for non-shared mapping, because we
> --
> 2.12.3
>


More information about the devel mailing list