You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/07/30 07:34:41 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1479: fs/vfs: Add file descriptor based events support
xiaoxiang781216 commented on a change in pull request #1479:
URL: https://github.com/apache/incubator-nuttx/pull/1479#discussion_r462760834
##########
File path: fs/vfs/fs_eventfd.c
##########
@@ -0,0 +1,623 @@
+/****************************************************************************
+ * vfs/fs_eventfd.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <stdio.h>
+#include <string.h>
+#include <poll.h>
+#include <errno.h>
+#include <fcntl.h>
+
+#include <debug.h>
+
+#include <sys/ioctl.h>
+#include <sys/eventfd.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define EVENTFD_FMT "/dev/efd%d"
+
+#ifndef CONFIG_EVENT_FD_NPOLLWAITERS
+/* Maximum number of threads than can be waiting for POLL events */
+# define CONFIG_EVENT_FD_NPOLLWAITERS 2
+#endif
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+/* This structure describes the internal state of the driver */
+
+struct eventfd_priv_s
+{
+ sem_t exclsem; /* Enforces exclusive access to device counter */
+ sem_t rdsem; /* Empty buffer - Reader waits for data write */
+ sem_t wrsem; /* Full buffer - Writer waits for data read */
+ uint8_t crefs; /* References counts on eventfd (limited to 255) */
+ uint8_t minor; /* eventfd minor number */
+ uint8_t mode_semaphore; /* eventfd mode (semaphore or counter) */
+ eventfd_t counter; /* eventfd counter */
Review comment:
After more thinking, since the writer also need wait when the count is too large, your implementation is reasonable.
##########
File path: fs/vfs/fs_eventfd.c
##########
@@ -0,0 +1,623 @@
+/****************************************************************************
+ * vfs/fs_eventfd.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <stdio.h>
+#include <string.h>
+#include <poll.h>
+#include <errno.h>
+#include <fcntl.h>
+
+#include <debug.h>
+
+#include <sys/ioctl.h>
+#include <sys/eventfd.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define EVENTFD_FMT "/dev/efd%d"
+
+#ifndef CONFIG_EVENT_FD_NPOLLWAITERS
+/* Maximum number of threads than can be waiting for POLL events */
+# define CONFIG_EVENT_FD_NPOLLWAITERS 2
+#endif
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+/* This structure describes the internal state of the driver */
+
+struct eventfd_priv_s
+{
+ sem_t exclsem; /* Enforces exclusive access to device counter */
+ sem_t rdsem; /* Empty buffer - Reader waits for data write */
+ sem_t wrsem; /* Full buffer - Writer waits for data read */
+ uint8_t crefs; /* References counts on eventfd (limited to 255) */
+ uint8_t minor; /* eventfd minor number */
+ uint8_t mode_semaphore; /* eventfd mode (semaphore or counter) */
+ eventfd_t counter; /* eventfd counter */
+
+ /* The following is a list if poll structures of threads waiting for
+ * driver events. The 'struct pollfd' reference for each open is also
+ * retained in the f_priv field of the 'struct file'.
+ */
+
+#ifdef CONFIG_EVENT_FD_POLL
+ FAR struct pollfd *d_fds[CONFIG_EVENT_FD_NPOLLWAITERS];
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int eventfd_open(FAR struct file *filep);
+static int eventfd_close(FAR struct file *filep);
+
+static ssize_t eventfd_do_read(FAR struct file *filep, FAR char *buffer,
+ size_t len);
+static ssize_t eventfd_do_write(FAR struct file *filep,
+ FAR const char *buffer, size_t len);
+static int eventfd_do_ioctl(FAR struct file *filep, int cmd,
+ unsigned long arg);
+#ifdef CONFIG_EVENT_FD_POLL
+static int eventfd_poll(FAR struct file *filep, FAR struct pollfd *fds,
+ bool setup);
+#endif
+
+static int eventfd_alloc_dev(eventfd_t counter, int flags);
+
+#ifdef CONFIG_EVENT_FD_POLL
+static void eventfd_pollnotify(FAR struct eventfd_priv_s *dev,
+ pollevent_t eventset);
+#endif
+
+static int get_unique_minor(void);
+static void release_minor(int minor);
+
+static FAR struct eventfd_priv_s *eventfd_allocdev(void);
+static void eventfd_destroy(FAR struct eventfd_priv_s *dev);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static const struct file_operations g_eventfd_fops =
+{
+ eventfd_open, /* open */
+ eventfd_close, /* close */
+ eventfd_do_read, /* read */
+ eventfd_do_write, /* write */
+ 0, /* seek */
+ eventfd_do_ioctl /* ioctl */
+#ifdef CONFIG_EVENT_FD_POLL
+ , eventfd_poll /* poll */
+#endif
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+FAR struct eventfd_priv_s *eventfd_allocdev(void)
+{
+ FAR struct eventfd_priv_s *dev;
+
+ dev = (FAR struct eventfd_priv_s *)
+ kmm_malloc(sizeof(struct eventfd_priv_s));
+ if (dev)
+ {
+ /* Initialize the private structure */
+
+ memset(dev, 0, sizeof(struct eventfd_priv_s));
+ nxsem_init(&dev->exclsem, 0, 0);
+ nxsem_init(&dev->rdsem, 0, 0);
+ nxsem_init(&dev->wrsem, 0, 0);
+
+ /* The read/write wait semaphores are used for signaling
+ * and should not have priority inheritance enabled.
+ */
+
+ nxsem_set_protocol(&dev->rdsem, SEM_PRIO_NONE);
+ nxsem_set_protocol(&dev->wrsem, SEM_PRIO_NONE);
+ }
+
+ return dev;
+}
+
+void eventfd_destroy(FAR struct eventfd_priv_s *dev)
+{
+ nxsem_destroy(&dev->exclsem);
+ nxsem_destroy(&dev->rdsem);
+ nxsem_destroy(&dev->wrsem);
+ kmm_free(dev);
+}
+
+#ifdef CONFIG_EVENT_FD_POLL
+void eventfd_pollnotify(FAR struct eventfd_priv_s *dev,
+ pollevent_t eventset)
+{
+ FAR struct pollfd *fds;
+ int i;
+
+ for (i = 0; i < CONFIG_EVENT_FD_NPOLLWAITERS; i++)
+ {
+ fds = dev->d_fds[i];
+ if (fds)
+ {
+ fds->revents |= eventset & fds->events;
+
+ if (fds->revents != 0)
+ {
+ nxsem_post(fds->sem);
+ }
+ }
+ }
+}
+#endif
+
+int get_unique_minor(void)
+{
+ static int minor = 0;
+
+ /* TODO reimplement this with minor bit map ? */
+
+ return (minor++) & 511;
Review comment:
since the minor in eventfd_priv_s is uint8_t, it si better to use 0xff.
##########
File path: fs/vfs/fs_eventfd.c
##########
@@ -0,0 +1,623 @@
+/****************************************************************************
+ * vfs/fs_eventfd.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <stdio.h>
+#include <string.h>
+#include <poll.h>
+#include <errno.h>
+#include <fcntl.h>
+
+#include <debug.h>
+
+#include <sys/ioctl.h>
+#include <sys/eventfd.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define EVENTFD_FMT "/dev/efd%d"
+
+#ifndef CONFIG_EVENT_FD_NPOLLWAITERS
+/* Maximum number of threads than can be waiting for POLL events */
+# define CONFIG_EVENT_FD_NPOLLWAITERS 2
+#endif
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+/* This structure describes the internal state of the driver */
+
+struct eventfd_priv_s
+{
+ sem_t exclsem; /* Enforces exclusive access to device counter */
+ sem_t rdsem; /* Empty buffer - Reader waits for data write */
+ sem_t wrsem; /* Full buffer - Writer waits for data read */
+ uint8_t crefs; /* References counts on eventfd (limited to 255) */
+ uint8_t minor; /* eventfd minor number */
+ uint8_t mode_semaphore; /* eventfd mode (semaphore or counter) */
+ eventfd_t counter; /* eventfd counter */
+
+ /* The following is a list if poll structures of threads waiting for
+ * driver events. The 'struct pollfd' reference for each open is also
+ * retained in the f_priv field of the 'struct file'.
+ */
+
+#ifdef CONFIG_EVENT_FD_POLL
+ FAR struct pollfd *d_fds[CONFIG_EVENT_FD_NPOLLWAITERS];
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int eventfd_open(FAR struct file *filep);
+static int eventfd_close(FAR struct file *filep);
+
+static ssize_t eventfd_do_read(FAR struct file *filep, FAR char *buffer,
+ size_t len);
+static ssize_t eventfd_do_write(FAR struct file *filep,
+ FAR const char *buffer, size_t len);
+static int eventfd_do_ioctl(FAR struct file *filep, int cmd,
+ unsigned long arg);
+#ifdef CONFIG_EVENT_FD_POLL
+static int eventfd_poll(FAR struct file *filep, FAR struct pollfd *fds,
+ bool setup);
+#endif
+
+static int eventfd_alloc_dev(eventfd_t counter, int flags);
+
+#ifdef CONFIG_EVENT_FD_POLL
+static void eventfd_pollnotify(FAR struct eventfd_priv_s *dev,
+ pollevent_t eventset);
+#endif
+
+static int get_unique_minor(void);
+static void release_minor(int minor);
+
+static FAR struct eventfd_priv_s *eventfd_allocdev(void);
+static void eventfd_destroy(FAR struct eventfd_priv_s *dev);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static const struct file_operations g_eventfd_fops =
+{
+ eventfd_open, /* open */
+ eventfd_close, /* close */
+ eventfd_do_read, /* read */
+ eventfd_do_write, /* write */
+ 0, /* seek */
+ eventfd_do_ioctl /* ioctl */
+#ifdef CONFIG_EVENT_FD_POLL
+ , eventfd_poll /* poll */
+#endif
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+FAR struct eventfd_priv_s *eventfd_allocdev(void)
+{
+ FAR struct eventfd_priv_s *dev;
+
+ dev = (FAR struct eventfd_priv_s *)
+ kmm_malloc(sizeof(struct eventfd_priv_s));
+ if (dev)
+ {
+ /* Initialize the private structure */
+
+ memset(dev, 0, sizeof(struct eventfd_priv_s));
+ nxsem_init(&dev->exclsem, 0, 0);
+ nxsem_init(&dev->rdsem, 0, 0);
+ nxsem_init(&dev->wrsem, 0, 0);
+
+ /* The read/write wait semaphores are used for signaling
+ * and should not have priority inheritance enabled.
+ */
+
+ nxsem_set_protocol(&dev->rdsem, SEM_PRIO_NONE);
+ nxsem_set_protocol(&dev->wrsem, SEM_PRIO_NONE);
+ }
+
+ return dev;
+}
+
+void eventfd_destroy(FAR struct eventfd_priv_s *dev)
Review comment:
Yes, even we add static for each function in "Private Function Prototypes" section, but the convention still add static in the implemetnation too.
##########
File path: fs/vfs/fs_eventfd.c
##########
@@ -0,0 +1,586 @@
+/****************************************************************************
+ * vfs/fs_eventfd.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <stdio.h>
+#include <string.h>
+#include <poll.h>
+#include <errno.h>
+#include <fcntl.h>
+
+#include <debug.h>
+
+#include <sys/ioctl.h>
+#include <sys/eventfd.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define EVENTFD_FMT "/dev/efd%d"
+
+#ifndef CONFIG_EVENT_FD_NPOLLWAITERS
+/* Maximum number of threads than can be waiting for POLL events */
+# define CONFIG_EVENT_FD_NPOLLWAITERS 2
+#endif
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+/* This structure describes the internal state of the driver */
+
+struct eventfd_priv_s
+{
+ sem_t exclsem; /* Enforces exclusive access to device counter */
+ sem_t rdsem; /* Empty buffer - Reader waits for data write */
+ sem_t wrsem; /* Full buffer - Writer waits for data read */
+ uint8_t crefs; /* References counts on eventfd (limited to 255) */
+ uint8_t minor; /* eventfd minor number */
+ uint8_t mode_semaphore; /* eventfd mode (semaphore or counter) */
+ eventfd_t counter; /* eventfd counter */
+
+ /* The following is a list if poll structures of threads waiting for
+ * driver events. The 'struct pollfd' reference for each open is also
+ * retained in the f_priv field of the 'struct file'.
+ */
+
+#ifdef CONFIG_EVENT_FD_POLL
+ FAR struct pollfd *d_fds[CONFIG_EVENT_FD_NPOLLWAITERS];
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int eventfd_open(FAR struct file *filep);
+static int eventfd_close(FAR struct file *filep);
+
+static ssize_t eventfd_do_read(FAR struct file *filep, FAR char *buffer,
+ size_t len);
+static ssize_t eventfd_do_write(FAR struct file *filep,
+ FAR const char *buffer, size_t len);
+static int eventfd_do_ioctl(FAR struct file *filep, int cmd,
+ unsigned long arg);
+#ifdef CONFIG_EVENT_FD_POLL
+static int eventfd_poll(FAR struct file *filep, FAR struct pollfd *fds,
+ bool setup);
+#endif
+
+#ifdef CONFIG_EVENT_FD_POLL
+static void eventfd_pollnotify(FAR struct eventfd_priv_s *dev,
+ pollevent_t eventset);
+#endif
+
+static int get_unique_minor(void);
+static void release_minor(int minor);
+
+static FAR struct eventfd_priv_s *eventfd_allocdev(void);
+static void eventfd_destroy(FAR struct eventfd_priv_s *dev);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static const struct file_operations g_eventfd_fops =
+{
+ eventfd_open, /* open */
+ eventfd_close, /* close */
+ eventfd_do_read, /* read */
+ eventfd_do_write, /* write */
+ 0, /* seek */
+ eventfd_do_ioctl /* ioctl */
+#ifdef CONFIG_EVENT_FD_POLL
+ , eventfd_poll /* poll */
+#endif
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+FAR struct eventfd_priv_s *eventfd_allocdev(void)
+{
+ FAR struct eventfd_priv_s *dev;
+
+ dev = (FAR struct eventfd_priv_s *)
+ kmm_malloc(sizeof(struct eventfd_priv_s));
+ if (dev)
+ {
+ /* Initialize the private structure */
+
+ memset(dev, 0, sizeof(struct eventfd_priv_s));
+ nxsem_init(&dev->exclsem, 0, 0);
+ nxsem_init(&dev->rdsem, 0, 0);
+ nxsem_init(&dev->wrsem, 0, 0);
+
+ /* The read/write wait semaphores are used for signaling
+ * and should not have priority inheritance enabled.
+ */
+
+ nxsem_set_protocol(&dev->rdsem, SEM_PRIO_NONE);
+ nxsem_set_protocol(&dev->wrsem, SEM_PRIO_NONE);
+ }
+
+ return dev;
+}
+
+void eventfd_destroy(FAR struct eventfd_priv_s *dev)
+{
+ nxsem_destroy(&dev->exclsem);
+ nxsem_destroy(&dev->rdsem);
+ nxsem_destroy(&dev->wrsem);
+ kmm_free(dev);
+}
+
+#ifdef CONFIG_EVENT_FD_POLL
+void eventfd_pollnotify(FAR struct eventfd_priv_s *dev,
+ pollevent_t eventset)
+{
+ FAR struct pollfd *fds;
+ int i;
+
+ for (i = 0; i < CONFIG_EVENT_FD_NPOLLWAITERS; i++)
+ {
+ fds = dev->d_fds[i];
+ if (fds)
+ {
+ fds->revents |= eventset & fds->events;
+
+ if (fds->revents != 0)
+ {
+ nxsem_post(fds->sem);
+ }
+ }
+ }
+}
+#endif
+
+int get_unique_minor(void)
+{
+ static int minor = 0;
+
+ /* TODO reimplement this with minor bit map ? */
+
+ return (minor++) & 511;
+}
+
+void release_minor(int minor)
+{
+}
+
+int eventfd_open(FAR struct file *filep)
+{
+ FAR struct inode *inode = filep->f_inode;
+ FAR struct eventfd_priv_s *priv = inode->i_private;
+ int ret;
+
+ /* Get exclusive access to the device structures */
+
+ ret = nxsem_wait(&priv->exclsem);
+ if (ret < 0)
+ {
+ return -1;
+ }
+
+ finfo("crefs: %d <%s>\n", priv->crefs, inode->i_name);
+
+ if (priv->crefs >= 255)
+ {
+ /* More than 255 opens; uint8_t would overflow to zero */
+
+ ret = -EMFILE;
+ }
+ else
+ {
+ /* Save the new open count on success */
+
+ priv->crefs += 1;
+ ret = OK;
+ }
+
+ nxsem_post(&priv->exclsem);
+ return ret;
+}
+
+int eventfd_close(FAR struct file *filep)
+{
+ FAR struct inode *inode = filep->f_inode;
+ FAR struct eventfd_priv_s *priv = inode->i_private;
+ char devpath[16];
+ int ret;
+
+ /* Get exclusive access to the device structures */
+
+ ret = nxsem_wait(&priv->exclsem);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ finfo("crefs: %d <%s>\n", priv->crefs, inode->i_name);
+
+ /* Decrement the references to the driver. If the reference count will
+ * decrement to 0, then uninitialize the driver.
+ */
+
+ if (priv->crefs > 1)
+ {
+ /* Just decrement the reference count and release the semaphore */
+
+ priv->crefs -= 1;
+ nxsem_post(&priv->exclsem);
+ return OK;
+ }
+
+ /* Re-create the path to the driver. */
+
+ finfo("destroy\n");
+ sprintf(devpath, EVENTFD_FMT, priv->minor);
+
+ /* Will be unregistered later after close is done */
+
+ unregister_driver(devpath);
+
+ DEBUGASSERT(priv->exclsem.semcount == 0);
+ release_minor(priv->minor);
+ eventfd_destroy(priv);
+
+ return OK;
+}
+
+ssize_t eventfd_do_read(FAR struct file *filep, FAR char *buffer, size_t len)
+{
+ FAR struct inode *inode = filep->f_inode;
+ FAR struct eventfd_priv_s *dev = inode->i_private;
+ ssize_t ret;
+ int sval;
+
+ if (len < sizeof(eventfd_t) || buffer == NULL)
+ {
+ return -EINVAL;
+ }
+
+ ret = nxsem_wait(&dev->exclsem);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ /* Wait for an incoming event */
+
+ while (dev->counter == 0)
+ {
+ if (filep->f_oflags & O_NONBLOCK)
+ {
+ nxsem_post(&dev->exclsem);
+ return -EAGAIN;
+ }
+
+ /* Wait for a writer to notify */
+
+ sched_lock();
Review comment:
sched_lock is unuseful in SMP config, it's better to remove sched_lock.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org