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.hu...@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
>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
