Support for multiplexed waiting on ach channels

[Tobias Furuholm]

Hi,

In another post you mention that you "ach does not currently support multiplexed waiting on ach channels". Do you have any plans on adding this in the future?

Regards,

Tobias Furuholm

[Neil T. Dantam]

Hi Tobias,

> In another post you mention that you "ach does not currently support
> multiplexed waiting on ach channels".

Yes, this is not possible using POSIX condition variables. A thread
can only wait on a single condition variable.

> Do you have any plans on adding this in the future?

Something like this might be possible with Linux futexes though it
hasn't made it to my immediate TODO list.

Cheers,
-ntd

> --
> You received this message because you are subscribed to the Google
> Groups "ach-devel" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to ach-devel+...@googlegroups.com
> <mailto:ach-devel+...@googlegroups.com>.
> For more options, visit https://groups.google.com/d/optout.

[Tobias Furuholm]

Hi Neil,

Thanks for the answer!

Read the source code and in ach.c I saw that there already was a comment about using futexes for waiting readers and writers. The same comment also mentions an idea on implementing notifications using eventfd. Is this something you are planning for or is this also further down the line?

/Tobias

[Neil T. Dantam]

Hi Tobias,

> using eventfd

Downside to this I think is that it requires an extra context switch.
One process waiting on the channel sees the new messages and notifies
others via eventfd. Not great for latency.

Probably the best way is to move the channels into the kernel, maybe
exposed as character devices. Then, receivers can select()/poll() on
multiple channels without an intermediate context-switch before
receiving a message.

> Is this something you are planning for or is this also further down
> the line?

It's not on my schedule for this month... but Dan Lofaro at over at
GMU may be able to find some students to work on it.

Dan, think you can find a student or two interested in taking this on
as a project? I'm happy to co-supervise.

Cheers,
-ntd

> > an email to ach-devel+...@googlegroups.com <javascript:>
> > <mailto:ach-devel+...@googlegroups.com <javascript:>>.

> > For more options, visit https://groups.google.com/d/optout

> <https://groups.google.com/d/optout>.

[Daniel Lofaro]

@Neil,

I will check with my students this week.  I expect ~40 hours of work total?

-Dan

an email to ach-devel+unsubscribe@googlegroups.com
<mailto:ach-devel+unsubscribe@googlegroups.com>.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "ach-devel" group.

To unsubscribe from this group and stop receiving emails from it, send an email to ach-devel+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Daniel M. Lofaro Ph.D
Assistant Professor, George Mason University 

Electrical and Computer Engineering 

Director of Lofaro Labs

e: d...@danLofaro.com

w: http://danLofaro.com

[Neil T. Dantam]

On 09/22/2014 10:56 AM, Daniel Lofaro wrote:
> I will check with my students this week.

Great!

> I expect ~40 hours of work total?

Highly dependent on programming experience. Maybe ~40 for good C
systems programmer, less for someone who's done kernel work before,
and more for someone with less experience. Also, there could be some
additional complexity, e.g., preempting partially-complete operations,
to integrate well with PREEMPT_RT (or Xenomai, if we want that in-kernel
too).

Thanks,
-ntd

> <mailto:ach-devel%2B...@googlegroups.com> <javascript:>
> > <mailto:ach-devel+...@__googlegroups.com
> <mailto:ach-devel%2B...@googlegroups.com> <javascript:>>.
> > For more options, visit
> https://groups.google.com/d/__optout
> <https://groups.google.com/d/optout>
> <https://groups.google.com/d/__optout

> <https://groups.google.com/d/optout>>.
>
> --
> You received this message because you are subscribed to the Google
> Groups "ach-devel" group.
> To unsubscribe from this group and stop receiving emails from
> it, send

> an email to ach-devel+unsubscribe@__googlegroups.com
> <mailto:ach-devel%2Bunsu...@googlegroups.com>
> <mailto:ach-devel+unsubscribe@__googlegroups.com
> <mailto:ach-devel%2Bunsu...@googlegroups.com>>.
> For more options, visit https://groups.google.com/d/__optout

> <https://groups.google.com/d/optout>.
>
>
> --
> You received this message because you are subscribed to the Google
> Groups "ach-devel" group.
> To unsubscribe from this group and stop receiving emails from it,

> send an email to ach-devel+unsubscribe@__googlegroups.com
> <mailto:ach-devel%2Bunsu...@googlegroups.com>.
> For more options, visit https://groups.google.com/d/__optout
> <https://groups.google.com/d/optout>.

>
>
>
>
> --
> Daniel M. Lofaro Ph.D
> Assistant Professor, George Mason University
> Electrical and Computer Engineering
> Director of Lofaro Labs
> e: d...@danLofaro.com

> w: http://danLofaro.com <http://danlofaro.com/>

>
> --
> You received this message because you are subscribed to the Google
> Groups "ach-devel" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to ach-devel+...@googlegroups.com

> <mailto:ach-devel+...@googlegroups.com>.

[Tobias Furuholm]

Awesome! I think this will be a great addition to ach...

[Kim Bøndergaard]

Hi

In agreement with Tobias I'll try to implement a kernel module providing ach channels  as character devices.

Basically I'll follow Neil's suggestion with one device (say /dev/achctrl) to be used for creating the actual channel devices.

My idea is to extend the ach create attributes with a flag, say map_kernel telling  the ach library to create the new channel as a kernel channel

The flag map_kernel and map_anon are of course mutual exclusive, and I surely would have preferred e.g. an enum, but due to backward compatibility...)

I.e. from an apps point of view creation of the channels remains almost as it is today.
 
When it comes to the other ach_ functions the api will remain the same.

The use case bringing up this issue was a need to listen on more channels at once from within one thread.
To support this feature a foresee a new function, say ach_get_fd(), able to handout the file descriptor behind a (kernel) ach_handle.

In order for the file descriptor to become 'ready' as expected when calling select() we'll probably need IOCTL means to set the right options before calling select() (probably ACH_O_WAIT)
When select() returns the app can use the existing ach_get() (probably with option ACH_O_LAST) to fetch the data.

Any comments on this?

/Kim

[Neil T. Dantam]

Hi Kim,

On 11/06/2014 05:19 AM, Kim Bøndergaard wrote:
> In agreement with Tobias I'll try to implement a kernel module providing
> ach channels as character devices.

Great!

> Basically I'll follow Neil's suggestion with one device (say
> /dev/achctrl) to be used for creating the actual channel devices.
>
> My idea is to extend the ach create attributes with a flag, say
> map_kernel telling the ach library to create the new channel as a
> kernel channel

Sounds good. I think it would be best to have a combined namespace
for user/kernel space channels -- so users can't create channels with
the same name in both user and kernel space, and ach_open() will open
from whichever space happens to exist.

> The flag map_kernel and map_anon are of course mutual exclusive, and I
> surely would have preferred e.g. an enum, but due to backward
> compatibility...)

Here's an option to mostly preserve compatibility:
* Replace the map_anon field with:
union { int map_anon; int map };
* Add:
enum ACH_MAP_SPACE
{
ACH_MAP_USER=0;
ACH_MAP_ANON=1;
ACH_MAP_KERNEL=2;

}

> I.e. from an apps point of view creation of the channels remains almost
> as it is today.
>
> When it comes to the other ach_ functions the api will remain the same.

Sounds good. Might make sense to implement the userpace/kernelspace
functions using a vtable?

> The use case bringing up this issue was a need to listen on more
> channels at once from within one thread.
> To support this feature a foresee a new function, say ach_get_fd(), able
> to handout the file descriptor behind a (kernel) ach_handle.

There is already an fd field in ach_channel_t, but might be better to
create the separate accessor function.

> In order for the file descriptor to become 'ready' as expected when
> calling select() we'll probably need IOCTL means to set the right
> options before calling select() (probably ACH_O_WAIT)
> When select() returns the app can use the existing ach_get() (probably
> with option ACH_O_LAST) to fetch the data.

I'd suggest doing this transparently as follows:
* Keep a field in userspace that tracks the kernel space flags set by
ioctl()
* When ach_get() is called, if the flags are different from what was
previously set, call the appropriate ioctl()

Cheers,
-ntd

[Kim Bøndergaard]

Hi Neil

I now have a kernel version running. Think it's time to get some feedback on my design.

Below a diff to your master commit 0566ec77d4bddad6eebb4ec5e8709c2e68ceee01. Would it be easier if I posted changes to e.g. gitorious ?

I haven't added vtables like suggested by you , but with the knowledge / overview I now have I can see  it might make sense to keep a vtable reference in the channel data.

When it comes to ach_get with expiration I've had to make a minor hack to improve performance.

Problem is that the expiration and read options have to be written via an ioctl().
To avoid calling ioctl() again and again in a typical ach_get()-loop I've cached expiration and options in the channel data, but due to expiration time being an abstime it does (typically) change for each time ach_get is called and thus forcing a new ioctl.
I've therefore defined that 'small' times (less than 10000 secs) are regarded as relative times. This makes it possible to define expiration to e.g. 3 sec in each ach_get(), thus avoiding the 'cached' data to become invalid.
Do you have any better solutions (another ach_get function, simply ignore the extra ioctl (in the end it is not that expensive) ...)?

In order to test my changes I've made a few changes in ach-bench.c as well.
I'll post the kernel module in a separate pos


diff --git a/include/ach.h b/include/ach.h
index 600d855..c998af3 100644
--- a/include/ach.h
+++ b/include/ach.h
@@ -229,6 +229,13 @@ extern "C" {
         ACH_O_COPY = 0x04
     } ach_get_opts_t;

+    typedef enum ach_map {
+       ACH_MAP_USER = 0,     /**< Use shared memory for channels */
+       ACH_MAP_ANON = 1,     /**< anonymous channel - use heap memory */
+       ACH_MAP_KERNEL = 2,   /**< Use kernel memory for channels - require ach kernel module being loaded */
+    } ach_map_t;
+
+
     /** Type for header in shared memory */
     struct ach_header;

@@ -236,19 +243,24 @@ extern "C" {
     typedef struct {
         union {
             struct{
-                int map_anon;        /**< anonymous channel (put it in process heap, not shm) */
+               union {
+                   int map_anon;        /**< anonymous channel (put it in process heap, not shm) */
+                   ach_map_t map;       /** < replaces map_anon */
+               };
                 struct ach_header *shm;   /**< the memory buffer used by anonymous channels */
             };
             uint64_t reserved_size[8]; /**< Reserve space to compatibly add future options */
         };
     } ach_attr_t;

-
     /** Attributes to pass to ach_create  */
     typedef struct {
         union {
             struct{
-                int map_anon;      /**< allocate channel in heap, rather than shm */
+               union {
+                   int map_anon;  /**< allocate channel in heap, rather than shm */
+                   ach_map_t map; /**< replaces map_anon */
+               };
                 struct ach_header *shm; /**< pointer to channel, set on output of create iff map_anon */
                 int truncate;      /**< remove and recreate an existing shm file */
                 int set_clock;     /**< if true, set the clock of the condition variable */
@@ -259,6 +271,12 @@ extern "C" {
         };
     } ach_create_attr_t;

+    /* Struct containing 'cache' of kernel module data to avoid updating when no changes exist */
+    typedef struct {
+       int options;
+       struct timespec reltime;   /**< kernel use relative time */
+    } achk_opt_t;
+
     /** Descriptor for shared memory area
      */
     typedef struct {
@@ -267,7 +285,10 @@ extern "C" {
                 struct ach_header *shm;   /**< pointer to mmap'ed block */
                 size_t len;          /**< length of memory mapping */
                 int fd;              /**< file descriptor of mmap'ed file */
-                uint64_t seq_num;    /**< last sequence number read */
+               union {
+                   uint64_t seq_num;    /**< last sequence number read */
+                   achk_opt_t k_opts;   /**< Used by kernel devices */
+               };
                 size_t next_index;   /**< next index entry to try get from */
                 ach_attr_t attr;     /**< attributes used to create this channel */
                 volatile sig_atomic_t cancel; /**< cancel a waiting ach_get */
diff --git a/include/ach_impl.h b/include/ach_impl.h
index 5687d22..8878b58 100644
--- a/include/ach_impl.h
+++ b/include/ach_impl.h
@@ -52,7 +52,15 @@ extern "C" {
 #endif

 /** prefix to apply to channel names to get the shared memory file name */
-#define ACH_CHAN_NAME_PREFIX "/achshm-"
+#define ACH_SHM_CHAN_NAME_PREFIX_PATH "/dev/shm/"
+#define ACH_SHM_CHAN_NAME_PREFIX_NAME "/achshm-"
+
+/** prefix to apply to channel names got get the chardev file name */
+#define ACH_CHAR_CHAN_NAME_PREFIX_PATH "/dev/"
+#define ACH_CHAR_CHAN_NAME_PREFIX_NAME "ach-"
+
+/** Name of chardev channel controlling device */
+#define ACH_CHAR_CHAN_CTRL_NAME "/dev/achctrl"

 /** Number of times to retry a syscall on EINTR before giving up */
 #define ACH_INTR_RETRY 8
diff --git a/src/ach.c b/src/ach.c
index cf7e793..e4241ac 100644
--- a/src/ach.c
+++ b/src/ach.c
@@ -68,7 +68,12 @@

 #include "ach.h"
 #include "ach_impl.h"
+#include "module/ach.h"
+#include <sys/wait.h>

+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/ioctl.h>

 #ifdef NDEBUG

@@ -84,6 +89,8 @@

 #endif /* NDEBUG */

+#define IS_KERNEL_DEVICE(chan)  (NULL == chan->shm)
+
 size_t ach_channel_size = sizeof(ach_channel_t);
 size_t ach_attr_size = sizeof(ach_attr_t);

@@ -167,14 +174,81 @@ static int channel_name_ok( const char *name ) {
 }

 static enum ach_status
+charfile_for_channel_name( const char *name, char *buf, size_t n ) {
+    if( n < ACH_CHAN_NAME_MAX + 16 ) return ACH_BUG;
+    if( !channel_name_ok(name)   ) return ACH_INVALID_NAME;
+    strcpy( buf, ACH_CHAR_CHAN_NAME_PREFIX_PATH);
+    strcat( buf, ACH_CHAR_CHAN_NAME_PREFIX_NAME);
+    strncat( buf, name, ACH_CHAN_NAME_MAX );
+    return ACH_OK;
+}
+
+static enum ach_status
 shmfile_for_channel_name( const char *name, char *buf, size_t n ) {
     if( n < ACH_CHAN_NAME_MAX + 16 ) return ACH_BUG;
     if( !channel_name_ok(name)   ) return ACH_INVALID_NAME;
-    strcpy( buf, ACH_CHAN_NAME_PREFIX );
+    strcpy( buf, ACH_SHM_CHAN_NAME_PREFIX_NAME );
     strncat( buf, name, ACH_CHAN_NAME_MAX );
     return ACH_OK;
 }

+static enum ach_status
+channame_to_full_shm_path(const char* channame, char *buf, size_t n) {
+    if (n < ACH_CHAN_NAME_MAX + 32) return ACH_BUG;
+    if (!channel_name_ok(channame) ) return ACH_INVALID_NAME;
+
+    strcpy(buf, ACH_SHM_CHAN_NAME_PREFIX_PATH);
+    strcat(buf, ACH_SHM_CHAN_NAME_PREFIX_NAME );
+    strncat(buf, channame, ACH_CHAN_NAME_MAX);
+    return ACH_OK;
+}
+
+static enum ach_status
+channel_exists_as_shm_device(const char* name)
+{
+    char ach_name[ACH_CHAN_NAME_MAX + 32];
+    int r = channame_to_full_shm_path(name, ach_name, sizeof(ach_name));
+    if (ACH_OK != r) return ACH_BUG;
+
+    struct stat buf;
+    if (stat(ach_name, &buf)) {
+       return ACH_ENOENT;
+    } else {
+       return ACH_OK;
+    }
+}
+
+
+static enum ach_status
+channel_exists_as_kernel_device(const char* name)
+{
+    char ach_name[ACH_CHAN_NAME_MAX + 16];
+    int r = charfile_for_channel_name(name, ach_name, sizeof(ach_name));
+    if (ACH_OK != r) return ACH_BUG;
+
+    struct stat buf;
+    if (stat (ach_name, &buf)) {
+       return ACH_ENOENT;
+    } else {
+       return ACH_OK;
+    }
+}
+
+static int fd_for_kernel_device_channel_name(const char *name,int oflag)
+{
+    char dev_name[ACH_CHAN_NAME_MAX+16];
+    int r = charfile_for_channel_name(name, dev_name, sizeof(dev_name));
+    if (ACH_OK != r) return -ACH_BUG;
+    int fd;
+    int i = 0;
+
+    do {
+        fd = open( dev_name, O_RDWR | oflag, 0666 );
+    }while( -1 == fd && EINTR == errno && i++ < ACH_INTR_RETRY);
+
+    return fd;
+}
+
 /** Opens shm file descriptor for a channel.
     \pre name is a valid channel name
 */
@@ -184,12 +258,179 @@ static int fd_for_channel_name( const char *name, int oflag ) {
     if( 0 != r ) return ACH_BUG;
     int fd;
     int i = 0;
+
     do {
         fd = shm_open( shm_name, O_RDWR | oflag, 0666 );
     }while( -1 == fd && EINTR == errno && i++ < ACH_INTR_RETRY);
     return fd;
 }

+/*****************************************************************
+  START charfile / kernel module helpers
+*****************************************************************/
+int charfile_unlink(const char* channel_name)
+{
+    int fd = open(ACH_CHAR_CHAN_CTRL_NAME, O_WRONLY|O_APPEND);
+    if (fd < 0) {
+       DEBUGF ("Failed opening kernel device controller\n");
+       return errno;
+    }
+    struct ach_ctrl_unlink_ch arg;
+    strcpy(arg.name, channel_name);
+
+    int ret = ioctl(fd, ACH_CTRL_UNLINK_CH, &arg);
+    if (ret < 0) {
+       ret = errno;
+       DEBUGF("Failed removing device %s\n", channel_name);
+    }
+
+    close(fd);
+    return ret;
+}
+
+enum ach_status
+achk_create( const char *channel_name,
+            size_t frame_cnt, size_t frame_size)
+{
+    if (ACH_OK == channel_exists_as_shm_device(channel_name))
+       return ACH_EEXIST;
+
+    int fd = open(ACH_CHAR_CHAN_CTRL_NAME, O_WRONLY|O_APPEND);
+    if (fd < 0) {
+       DEBUGF("Failed opening kernel device controller\n");
+       return ACH_FAILED_SYSCALL;
+    }
+    struct ach_ctrl_create_ch arg;
+    arg.frame_cnt = frame_cnt;
+    arg.frame_size = frame_size;
+    strcpy(arg.name, channel_name);
+
+    enum ach_status ach_stat = ACH_OK;
+    int ret = ioctl(fd, ACH_CTRL_CREATE_CH, &arg);
+    if (ret < 0) {
+       DEBUGF("Failed creating device %s\n", channel_name);
+       ach_stat = check_errno();
+    }
+
+    close(fd);
+    return ach_stat;
+}
+
+static struct timespec relative_time(struct timespec then)
+{
+  struct timespec delta;
+  struct timespec now;
+
+  clock_gettime( ACH_DEFAULT_CLOCK, &now );
+
+  delta.tv_sec = 0;
+  delta.tv_nsec = 0;
+
+  if (now.tv_sec > then.tv_sec)
+      return delta;
+  if (now.tv_sec == then.tv_sec &&
+      now.tv_nsec > then.tv_nsec)
+      return delta;
+  delta.tv_sec = then.tv_sec - now.tv_sec;
+  if (now.tv_nsec > then.tv_nsec) {
+      delta.tv_sec--;
+      then.tv_nsec += 1e9;
+  }
+  delta.tv_nsec = then.tv_nsec - now.tv_sec;
+  return delta;
+}
+
+static enum ach_status
+achk_get( ach_channel_t *chan,
+          void *cx, char **pobj,
+          size_t *frame_size,
+          const struct timespec *ACH_RESTRICT abstime,
+          int options )
+{
+    size_t size = *(size_t*)cx;
+    achk_opt_t opts;
+    opts.options = options;
+    if(abstime)
+       opts.reltime =*abstime;
+    else {
+       opts.reltime.tv_sec = 0;
+       opts.reltime.tv_nsec = 0;
+    }
+
+    if (opts.reltime.tv_sec > 10000) {
+       /* Regard time as abstime - convert to relative time */
+       opts.reltime = relative_time(opts.reltime);
+    }
+    if (memcmp(&opts, &chan->k_opts, sizeof(achk_opt_t))) {
+       struct ach_ch_mode mode;
+       memset(&mode, 0, sizeof(mode));
+
+       if (options & ACH_O_WAIT) mode.mode |= ACH_CH_MODE_WAIT;
+       if (options & ACH_O_LAST) mode.mode |= ACH_CH_MODE_LAST;
+       if (options & ACH_O_COPY) mode.mode |= ACH_CH_MODE_COPY;
+
+       mode.reltime = opts.reltime;
+
+       if (ioctl(chan->fd, ACH_CH_SET_MODE, &mode)) {
+           return ACH_FAILED_SYSCALL;
+       }
+       chan->k_opts = opts;
+    }
+
+    ssize_t ret = read(chan->fd, *pobj, size);
+    if ( ret < 0) {
+       switch (errno) {
+       case EAGAIN: return ACH_STALE_FRAMES; break;
+       case ETIME: return ACH_TIMEOUT; break;
+       case ECANCELED: return ACH_CANCELED; break;
+       default: return ACH_FAILED_SYSCALL; break;
+       }
+    }
+    *frame_size = ret;
+    return ACH_OK;
+}
+
+static enum ach_status
+achk_put( ach_channel_t *chan,
+          void *cx, const char *obj, size_t len )
+{
+    ssize_t size = write(chan->fd, obj, len);
+    if (size < 0)
+       return check_errno();
+    return ACH_OK;
+}
+
+static enum ach_status
+achk_flush( ach_channel_t * chan)
+{
+    unsigned int arg = 0;
+    if (ioctl(chan->fd, ACH_CH_FLUSH, arg)) {
+       return ACH_FAILED_SYSCALL;
+    }
+    return ACH_OK;
+}
+
+enum ach_status
+achk_cancel( ach_channel_t *chan, const ach_cancel_attr_t *attr ) {
+
+    unsigned int arg;
+
+    if (!attr)
+       arg = 0;
+    else
+       arg = attr->async_unsafe ? ACH_CH_CANCEL_UNSAFE : 0;
+
+    chan->cancel = 1;
+    if (ioctl(chan->fd, ACH_CH_CANCEL, arg)) {
+       return ACH_FAILED_SYSCALL;
+    }
+    return ACH_OK;
+}
+
+/*****************************************************************
+  END charfile / kernel module helpers
+*****************************************************************/
+


 /*! \page synchronization Synchronization
@@ -349,6 +590,21 @@ ach_create( const char *channel_name,
     ach_header_t *shm;
     int fd;
     size_t len;
+
+    if (attr && ACH_MAP_KERNEL == attr->map) {
+       enum ach_status r =  achk_create(channel_name, frame_cnt, frame_size);
+       if (ACH_OK == r) {
+           int retry = 0;
+           /* Wait for device to become ready */
+           r = channel_exists_as_kernel_device(channel_name);
+           while ( (ACH_OK != r) && (retry++ < ACH_INTR_RETRY*10)) {
+                   usleep(1000);
+                   r = channel_exists_as_kernel_device(channel_name);
+           }
+       }
+       return r;
+    }
+
     /* fixme: truncate */
     /* open shm */
     {
@@ -539,6 +795,24 @@ ach_open( ach_channel_t *chan, const char *channel_name,
     size_t len;
     int fd = -1;

+    if ((attr && ACH_MAP_KERNEL == attr->map) ||
+       ACH_OK == channel_exists_as_kernel_device(channel_name)) {
+
+      if ( (fd = fd_for_kernel_device_channel_name(channel_name,0)) < 0 ) {
+       return check_errno();
+      }
+
+      /* initialize struct */
+      chan->fd = fd;
+      chan->len = 0;        /* We don't care */
+      chan->shm = NULL;     /* Indicates kernel device */
+      chan->seq_num = 0;    /* Not used */
+      chan->next_index = 0; /* Not used */
+      chan->cancel = 0;
+
+      return ACH_OK;
+    }
+
     if( attr ) memcpy( &chan->attr, attr, sizeof(chan->attr) );
     else memset( &chan->attr, 0, sizeof(chan->attr) );

@@ -549,7 +823,6 @@ ach_open( ach_channel_t *chan, const char *channel_name,
         if( ! channel_name_ok( channel_name ) )
             return ACH_INVALID_NAME;
         /* open shm */
-        if( ! channel_name_ok( channel_name ) ) return ACH_INVALID_NAME;
         if( (fd = fd_for_channel_name( channel_name, 0 )) < 0 ) {
             return check_errno();
         }
@@ -611,6 +884,9 @@ ach_get( ach_channel_t *chan, void *buf, size_t size,
          const struct timespec *ACH_RESTRICT abstime,
          int options )
 {
+    if (IS_KERNEL_DEVICE(chan)) {
+       return achk_get( chan, &size, (char**)&buf, frame_size, abstime, options);
+    }
     return ach_xget( chan,
                      get_fun, &size, &buf,
                      frame_size, abstime, options );
@@ -619,6 +895,11 @@ ach_get( ach_channel_t *chan, void *buf, size_t size,

 enum ach_status
 ach_flush( ach_channel_t *chan ) {
+
+    if (IS_KERNEL_DEVICE(chan)) {
+       return achk_flush(chan);
+    }
+
     ach_header_t *shm = chan->shm;
     enum ach_status r = rdlock(chan, 0,  NULL);
     if( ACH_OK != r ) return r;
@@ -656,6 +937,9 @@ put_fun(void *cx, void *chan_dst, const void *obj)
 enum ach_status
 ach_put( ach_channel_t *chan, const void *buf, size_t len )
 {
+    if (IS_KERNEL_DEVICE(chan)) {
+       return achk_put( chan, &len, buf, len);
+    }
     return ach_xput( chan, put_fun, &len, buf, len );
 }

@@ -728,14 +1012,23 @@ ach_chmod( ach_channel_t *chan, mode_t mode ) {
     return (0 == fchmod(chan->fd, mode)) ? ACH_OK : check_errno();;
 }

-
 enum ach_status
 ach_unlink( const char *name ) {
-    char shm_name[ACH_CHAN_NAME_MAX + 16];
-    enum ach_status r = shmfile_for_channel_name( name, shm_name, sizeof(shm_name) );
+    char ach_name[ACH_CHAN_NAME_MAX + 16];
+    enum ach_status kstat = channel_exists_as_kernel_device(name);
+
+    if (ACH_ENOENT == channel_exists_as_shm_device(name) &&
+       ACH_ENOENT == kstat) {
+       return ACH_ENOENT;
+    }
+    if (ACH_OK == kstat) {
+       enum ach_status r = charfile_unlink(name);
+       return r;
+    }
+
+    enum ach_status r = shmfile_for_channel_name(name, ach_name, sizeof(ach_name));
     if( ACH_OK == r ) {
-        /*r = shm_unlink(name); */
-        int i = shm_unlink(shm_name);
+       int i = shm_unlink(ach_name);
         if( 0 == i ) {
             return  ACH_OK;
         } else {
@@ -758,6 +1051,10 @@ enum ach_status
 ach_cancel( ach_channel_t *chan, const ach_cancel_attr_t *attr ) {
     if( NULL == attr ) attr = &default_cancel_attr;

+    if (IS_KERNEL_DEVICE(chan)) {
+       return achk_cancel(chan, attr);
+    }
+
     if( attr->async_unsafe ) {
         /* Don't be async safe, i.e., called from another thread */
         enum ach_status r = chan_lock(chan);

[Neil Dantam]

Great! Would you mind putting this up in a publicly accessible git
repo?

I am traveling this week, so probably can't do a detailed review
till the weekend.

> When it comes to ach_get with expiration I've had to make a minor
> hack to improve performance.

Hmm, this is a little unsatisfying... Is there a poll-like system
call that takes an absolute timespec? Looks like ppoll() and
pselect() both take relative times. The extra ioctl() is probably
not too much overhead. Could also add a RELATIVE_TIME option to
ach_get().

Cheers,
-ntd

[Kim Bøndergaard]

Hi Neil

My changes are now available at github in https://github.com/KimBP/ach.git (branch upstream)

Regarding the relative times I think we might simply ignore it as an option. It does degrade performance a little but due to the fact that the whole idea with kernel channels was to provide means to concurrent wait (a.k.a select() ) on more channels at once and the fact that select() does by it self provide a timeout option, calling ach_get() with timeout probably isn't used in practice.

When using select() the drivers poll function is by the way called once when select() is called and (I think) again when a 'selected' file descriptor becomes ready.

/Kim

[Tobias Furuholm]

Hi Neil and Kim,

Would it make sense to only support blocking read in the kernel module and implement the timeout by using select in the userspace part of the library?

/Tobias

[Kim Bøndergaard]

Like I see it there'll be no advantages of making that constraint.

I'd prefer to keep the feature set alike for the two modes. This way it becomes much simpler to change design if you by some reason want to do that.

If by some reason (performance?) seems to become a problem with timers in achget() on a kernel channel you can just go for the blocking read without timeout. It's an optional feature you know.

Again, I think your typical usage would be

while (
   select()

   if (FD_SET()  )

[Kim Bøndergaard]

Sorry pressed submit too early. See below

Den fredag den 21. november 2014 07.15.24 UTC+1 skrev Kim Bøndergaard:

Like I see it there'll be no advantages of making that constraint.

I'd prefer to keep the feature set alike for the two modes. This way it becomes much simpler to change design if you by some reason want to do that.

If by some reason (performance?) seems to become a problem with timers in achget() on a kernel channel you can just go for the blocking read without timeout. It's an optional feature you know.

Again, I think your typical usage with kernel channels would be

while (
   select()

   if (FD_SET()  )

       achget(NO_WAIT)

    }

    But way put the constraint with lack on timeout on it. (Logging thread or similar)