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 2023/01/06 12:59:48 UTC
[GitHub] [nuttx] XinStellaris opened a new pull request, #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN threshold
XinStellaris opened a new pull request, #8052:
URL: https://github.com/apache/nuttx/pull/8052
## Summary
add a new IOC command PIPEIOC_POLLTHRES to set POLLIN threshold.
When local udp socket is used, this threshold will avoid non-blocking read failed with -EAGAIN when only preamble len is sent and read by reader. To do so, local udp socket should set threshold to be sizeof preamble len.
Setting local udp socket threshold will be done in another patch.
## Impact
None, only affects others after using the new command.
## Testing
Tested with CONFIG_EXAMPLES_UDGRAM
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN/POLLOUT threshold
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#discussion_r1065348318
##########
drivers/pipes/pipe_common.c:
##########
@@ -60,6 +60,26 @@
# define pipe_dumpbuffer(m,a,n)
#endif
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pipecommon_bufferused
+ ****************************************************************************/
+
+static pipe_ndx_t pipecommon_bufferused(FAR struct pipe_dev_s *dev)
Review Comment:
Ok
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] XinStellaris commented on a diff in pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN/POLLOUT threshold
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on code in PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#discussion_r1065272889
##########
drivers/pipes/pipe_common.c:
##########
@@ -772,6 +796,20 @@ int pipecommon_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
}
break;
+ case PIPEIOC_POLLTHRES:
+ {
+ pipe_ndx_t threshold = (pipe_ndx_t)arg;
Review Comment:
Okay, it will be done.
Current code is okay for our case, but from user's view, it is better to set different threshold for input/output so that it can be used in other places
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN threshold
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#issuecomment-1373753775
@XinStellaris I noticed that only the pipe2() has a good/complete Description comment, could you please extent the Description to get it could complete? It could help more people studying the NuttX source code!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] XinStellaris commented on a diff in pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN/POLLOUT threshold
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on code in PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#discussion_r1065310572
##########
drivers/pipes/pipe_common.c:
##########
@@ -60,6 +60,26 @@
# define pipe_dumpbuffer(m,a,n)
#endif
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pipecommon_bufferused
+ ****************************************************************************/
+
+static pipe_ndx_t pipecommon_bufferused(FAR struct pipe_dev_s *dev)
Review Comment:
In my opinion, the POLLOUT handling is simple enough. Adding a new function may help, but it seems like an overkill.
Also, in pipecommon_poll:
nbytes = pipecommon_bufferused(dev);
/* Notify the POLLOUT event if the pipe buffer can accept
* more than d_polloutthrd bytes, but only if
* there is readers.
*/
eventset = 0;
if ((filep->f_oflags & O_WROK) &&
nbytes < (dev->d_bufsize - dev->d_polloutthrd))
{
eventset |= POLLOUT;
}
/* Notify the POLLIN event if buffer used exceeds poll threshold */
if ((filep->f_oflags & O_RDOK) && (nbytes > dev->d_pollinthrd))
{
eventset |= POLLIN;
}
/* Notify the POLLHUP event if the pipe is empty and no writers */
if (nbytes == 0 && dev->d_nwriters <= 0)
{
eventset |= POLLHUP;
}
Adding a new pipecommon_bufferleft will lead to calculating buffer used bytes twice, since it is already done in pipecommon_bufferused. A little redundant?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] XinStellaris commented on a diff in pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN threshold
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on code in PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#discussion_r1064417315
##########
drivers/pipes/pipe_common.h:
##########
@@ -120,6 +120,7 @@ struct pipe_dev_s
pipe_ndx_t d_wrndx; /* Index in d_buffer to save next byte written */
pipe_ndx_t d_rdndx; /* Index in d_buffer to return the next byte read */
pipe_ndx_t d_bufsize; /* allocated size of d_buffer in bytes */
+ pipe_ndx_t d_pollthres; /* Buffer threshold for POLLIN to occur */
Review Comment:
Yes, after some thinking, I think it is necessary.
We should avoid writing when only preamble can be written.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN/POLLOUT threshold
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#discussion_r1065301455
##########
include/nuttx/fs/ioctl.h:
##########
@@ -405,15 +405,33 @@
/* FIFOs and pipe driver ioctl definitions **********************************/
-#define _PIPEIOCVALID(c) (_IOC_TYPE(c)==_PIPEBASE)
-#define _PIPEIOC(nr) _IOC(_PIPEBASE,nr)
-
-#define PIPEIOC_POLICY _PIPEIOC(0x0001) /* Set buffer policy
- * IN: unsigned long integer
- * 0=free on last close
- * (default)
- * 1=fre when empty
- * OUT: None */
+#define _PIPEIOCVALID(c) (_IOC_TYPE(c)==_PIPEBASE)
+#define _PIPEIOC(nr) _IOC(_PIPEBASE,nr)
+
+#define PIPEIOC_POLICY _PIPEIOC(0x0001) /* Set buffer policy
+ * IN: unsigned long integer
+ * 0=free on last close
+ * (default)
+ * 1=fre when empty
+ * OUT: None */
+
+#define PIPEIOC_POLLINTHRD _PIPEIOC(0x0002) /* Set pipe POLLIN
+ * notifty buffer threshold.
+ * IN: unsigned long integer.
+ * POLLIN only occurs when
+ * buffer contains more
Review Comment:
should we use >= for PIPEIOC_POLLINTHRD and PIPEIOC_POLLOUTTHRD and change the default valu of d_polloutthrd and d_pollinthrd to 1?
##########
drivers/pipes/pipe_common.c:
##########
@@ -60,6 +60,26 @@
# define pipe_dumpbuffer(m,a,n)
#endif
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pipecommon_bufferused
+ ****************************************************************************/
+
+static pipe_ndx_t pipecommon_bufferused(FAR struct pipe_dev_s *dev)
Review Comment:
let' add pipecommon_bufferleft to simplify POLLOUT handing
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] XinStellaris commented on pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN threshold
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#issuecomment-1375316111
> @XinStellaris I noticed that only the pipe2() has a good/complete Description comment, could you please extent the Description to get it could complete? It could help more people studying the NuttX source code!
Could you point out which file/function need more description?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN/POLLOUT threshold
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#discussion_r1064879886
##########
drivers/pipes/pipe_common.c:
##########
@@ -772,6 +796,20 @@ int pipecommon_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
}
break;
+ case PIPEIOC_POLLTHRES:
+ {
+ pipe_ndx_t threshold = (pipe_ndx_t)arg;
Review Comment:
should we support the different threshold for input/output? to avoid the reader and writer interference each other
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN/POLLOUT threshold
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#discussion_r1065317939
##########
include/nuttx/fs/ioctl.h:
##########
@@ -405,15 +405,33 @@
/* FIFOs and pipe driver ioctl definitions **********************************/
-#define _PIPEIOCVALID(c) (_IOC_TYPE(c)==_PIPEBASE)
-#define _PIPEIOC(nr) _IOC(_PIPEBASE,nr)
-
-#define PIPEIOC_POLICY _PIPEIOC(0x0001) /* Set buffer policy
- * IN: unsigned long integer
- * 0=free on last close
- * (default)
- * 1=fre when empty
- * OUT: None */
+#define _PIPEIOCVALID(c) (_IOC_TYPE(c)==_PIPEBASE)
+#define _PIPEIOC(nr) _IOC(_PIPEBASE,nr)
+
+#define PIPEIOC_POLICY _PIPEIOC(0x0001) /* Set buffer policy
+ * IN: unsigned long integer
+ * 0=free on last close
+ * (default)
+ * 1=fre when empty
+ * OUT: None */
+
+#define PIPEIOC_POLLINTHRD _PIPEIOC(0x0002) /* Set pipe POLLIN
+ * notifty buffer threshold.
+ * IN: unsigned long integer.
+ * POLLIN only occurs when
+ * buffer contains more
Review Comment:
Ok
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] XinStellaris commented on a diff in pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN/POLLOUT threshold
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on code in PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#discussion_r1065307647
##########
include/nuttx/fs/ioctl.h:
##########
@@ -405,15 +405,33 @@
/* FIFOs and pipe driver ioctl definitions **********************************/
-#define _PIPEIOCVALID(c) (_IOC_TYPE(c)==_PIPEBASE)
-#define _PIPEIOC(nr) _IOC(_PIPEBASE,nr)
-
-#define PIPEIOC_POLICY _PIPEIOC(0x0001) /* Set buffer policy
- * IN: unsigned long integer
- * 0=free on last close
- * (default)
- * 1=fre when empty
- * OUT: None */
+#define _PIPEIOCVALID(c) (_IOC_TYPE(c)==_PIPEBASE)
+#define _PIPEIOC(nr) _IOC(_PIPEBASE,nr)
+
+#define PIPEIOC_POLICY _PIPEIOC(0x0001) /* Set buffer policy
+ * IN: unsigned long integer
+ * 0=free on last close
+ * (default)
+ * 1=fre when empty
+ * OUT: None */
+
+#define PIPEIOC_POLLINTHRD _PIPEIOC(0x0002) /* Set pipe POLLIN
+ * notifty buffer threshold.
+ * IN: unsigned long integer.
+ * POLLIN only occurs when
+ * buffer contains more
Review Comment:
I think > for POLLIN/POLLOUT threshold is more reasonable and readable. For example, in local udp socket, we should avoid triggering POLLIN when buffered content is the size of preamble, that is "more than" logic in its nature.
Though > and >= both can be implemented, and I can replace it with >= and change default value to 1. I don't think ">=" is more readable.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] XinStellaris commented on a diff in pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN/POLLOUT threshold
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on code in PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#discussion_r1065310572
##########
drivers/pipes/pipe_common.c:
##########
@@ -60,6 +60,26 @@
# define pipe_dumpbuffer(m,a,n)
#endif
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pipecommon_bufferused
+ ****************************************************************************/
+
+static pipe_ndx_t pipecommon_bufferused(FAR struct pipe_dev_s *dev)
Review Comment:
In my opinion, the POLLOUT handling is simple enough. Adding a new function may help, but it seems like an overkill.
Also, in pipecommon_iotcl:
nbytes = pipecommon_bufferused(dev);
/* Notify the POLLOUT event if the pipe buffer can accept
* more than d_polloutthrd bytes, but only if
* there is readers.
*/
eventset = 0;
if ((filep->f_oflags & O_WROK) &&
nbytes < (dev->d_bufsize - dev->d_polloutthrd))
{
eventset |= POLLOUT;
}
/* Notify the POLLIN event if buffer used exceeds poll threshold */
if ((filep->f_oflags & O_RDOK) && (nbytes > dev->d_pollinthrd))
{
eventset |= POLLIN;
}
/* Notify the POLLHUP event if the pipe is empty and no writers */
if (nbytes == 0 && dev->d_nwriters <= 0)
{
eventset |= POLLHUP;
}
Adding a new pipecommon_bufferleft will lead to calculating buffer used bytes twice, since it is already done in pipecommon_bufferused. A little redundant?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 merged pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN/POLLOUT threshold
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #8052:
URL: https://github.com/apache/nuttx/pull/8052
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8052: drivers/pipe:add PIPEIOC_POLLTHRES to set POLLIN threshold
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8052:
URL: https://github.com/apache/nuttx/pull/8052#discussion_r1063481601
##########
drivers/pipes/pipe_common.h:
##########
@@ -120,6 +120,7 @@ struct pipe_dev_s
pipe_ndx_t d_wrndx; /* Index in d_buffer to save next byte written */
pipe_ndx_t d_rdndx; /* Index in d_buffer to return the next byte read */
pipe_ndx_t d_bufsize; /* allocated size of d_buffer in bytes */
+ pipe_ndx_t d_pollthres; /* Buffer threshold for POLLIN to occur */
Review Comment:
should we support POLLOUT direction too
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org