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 2021/07/06 09:19:35 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #4080: net/socket: add SO_SNDBUF support
anchao opened a new pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080
## Summary
net/socket: add SO_SNDBUF support
net/wrbuffer: add tcp/udp_wrbuffer_inqueue_size() interface
Reference here:
https://www.freebsd.org/cgi/man.cgi?query=setsockopt&sektion=2&format=html
## Impact
add SO_SNDBUF support to limit the the sharing of iob buffers usage under multiple TCP/UDP connections
## Testing
iperf client with multiple instance to run the net performance at same time
--
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] [incubator-nuttx] anchao commented on pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#issuecomment-883343485
@yamt san, please review again
--
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] [incubator-nuttx] yamt commented on a change in pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#discussion_r664945030
##########
File path: net/Kconfig
##########
@@ -123,6 +123,12 @@ config NET_RECV_BUFSIZE
---help---
This is the default value for receive buffer size.
+config NET_SEND_BUFSIZE
Review comment:
maybe depends on NET_TCP_WRITE_BUFFERS?
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#discussion_r671102618
##########
File path: net/udp/udp_sendto_buffered.c
##########
@@ -657,6 +671,23 @@ ssize_t psock_udp_sendto(FAR struct socket *psock, FAR const void *buf,
{
net_lock();
+#if CONFIG_NET_SEND_BUFSIZE > 0
+ /* If the send buffer size exceeds the send limit,
+ * wait for the write buffer to be released
+ */
+
+ while (udp_inqueue_wrb_size(conn) >= conn->sndbufs)
Review comment:
add + len
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1098,6 +1122,23 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
psock->s_sndcb->priv = (FAR void *)psock;
psock->s_sndcb->event = psock_send_eventhandler;
+#if CONFIG_NET_SEND_BUFSIZE > 0
+ /* If the send buffer size exceeds the send limit,
+ * wait for the write buffer to be released
+ */
+
+ while (tcp_inqueue_wrb_size(conn) >= conn->snd_bufs)
Review comment:
+ len
--
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] [incubator-nuttx] yamt commented on a change in pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#discussion_r664939462
##########
File path: net/tcp/tcp_wrbuffer.c
##########
@@ -152,17 +152,31 @@ uint32_t tcp_wrbuffer_inqueue_size(FAR struct tcp_conn_s *conn)
* of TCP data is about to sent
*
* Input Parameters:
- * None
+ * conn - The TCP connection of interest
*
* Assumptions:
* Called from user logic with the network locked.
*
****************************************************************************/
-FAR struct tcp_wrbuffer_s *tcp_wrbuffer_alloc(void)
+FAR struct tcp_wrbuffer_s *tcp_wrbuffer_alloc(FAR struct tcp_conn_s *conn)
{
FAR struct tcp_wrbuffer_s *wrb;
+#if CONFIG_NET_SEND_BUFSIZE > 0
+ if (conn)
+ {
+ /* If the send buffer size exceeds the send limit, wait for the write
+ * buffer to be released
+ */
+
+ while (tcp_wrbuffer_inqueue_size(conn) > conn->snd_bufs)
+ {
+ net_lockedwait_uninterruptible(&conn->snd_sem);
Review comment:
i guess this kind of things should be interruptible. but it isn't a new problem in this change.
##########
File path: net/tcp/tcp_wrbuffer.c
##########
@@ -105,6 +105,44 @@ void tcp_wrbuffer_initialize(void)
nxsem_set_protocol(&g_wrbuffer.sem, SEM_PRIO_NONE);
}
+/****************************************************************************
+ * Name: tcp_wrbuffer_inqueue_size
+ *
+ * Description:
+ * Get the in-queued write buffer size from connection
+ *
+ * Input Parameters:
+ * conn - The TCP connection of interest
+ *
+ * Assumptions:
+ * Called from user logic with the network locked.
+ *
+ ****************************************************************************/
+
+uint32_t tcp_wrbuffer_inqueue_size(FAR struct tcp_conn_s *conn)
+{
+ FAR struct tcp_wrbuffer_s *wrb;
+ FAR sq_entry_t *entry;
+ uint32_t total = 0;
+
+ if (conn)
+ {
+ for (entry = sq_peek(&conn->unacked_q); entry; entry = sq_next(entry))
+ {
+ wrb = (FAR struct tcp_wrbuffer_s *)entry;
+ total += TCP_WBSENT(wrb);
+ }
+
+ for (entry = sq_peek(&conn->write_q); entry; entry = sq_next(entry))
+ {
+ wrb = (FAR struct tcp_wrbuffer_s *)entry;
+ total += TCP_WBSENT(wrb);
Review comment:
TCP_WBSENT is on-wire bytes, which is not appropriate thing to look at for SO_SNDBUF.
i guess TCP_WBPKTLEN is more appropriate. how do you think?
--
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] [incubator-nuttx] anchao commented on a change in pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#discussion_r665013358
##########
File path: net/tcp/tcp_wrbuffer.c
##########
@@ -105,6 +105,44 @@ void tcp_wrbuffer_initialize(void)
nxsem_set_protocol(&g_wrbuffer.sem, SEM_PRIO_NONE);
}
+/****************************************************************************
+ * Name: tcp_wrbuffer_inqueue_size
+ *
+ * Description:
+ * Get the in-queued write buffer size from connection
+ *
+ * Input Parameters:
+ * conn - The TCP connection of interest
+ *
+ * Assumptions:
+ * Called from user logic with the network locked.
+ *
+ ****************************************************************************/
+
+uint32_t tcp_wrbuffer_inqueue_size(FAR struct tcp_conn_s *conn)
+{
+ FAR struct tcp_wrbuffer_s *wrb;
+ FAR sq_entry_t *entry;
+ uint32_t total = 0;
+
+ if (conn)
+ {
+ for (entry = sq_peek(&conn->unacked_q); entry; entry = sq_next(entry))
+ {
+ wrb = (FAR struct tcp_wrbuffer_s *)entry;
+ total += TCP_WBSENT(wrb);
+ }
+
+ for (entry = sq_peek(&conn->write_q); entry; entry = sq_next(entry))
+ {
+ wrb = (FAR struct tcp_wrbuffer_s *)entry;
+ total += TCP_WBSENT(wrb);
Review comment:
Done
##########
File path: net/Kconfig
##########
@@ -123,6 +123,12 @@ config NET_RECV_BUFSIZE
---help---
This is the default value for receive buffer size.
+config NET_SEND_BUFSIZE
Review comment:
Done
--
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] [incubator-nuttx] anchao commented on pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#issuecomment-879787044
> i'm not sure what you mean here. can you explain a bit?
If we move the check outside the wrbuffer, we will duplicate a lot of sem_post codes in these places. Do you want to do this? I don't think there is any place like net/tcp/tcp_wrbuffer.c is more suitable.
```
net/tcp/tcp_send_buffered.c:214: tcp_wrbuffer_release((FAR struct tcp_wrbuffer_s *)entry);
net/tcp/tcp_send_buffered.c:220: tcp_wrbuffer_release((FAR struct tcp_wrbuffer_s *)entry);
net/tcp/tcp_send_buffered.c:434: tcp_wrbuffer_release(wrb);
net/tcp/tcp_send_buffered.c:607: tcp_wrbuffer_release(wrb);
net/tcp/tcp_send_buffered.c:672: tcp_wrbuffer_release(wrb);
net/tcp/tcp_send_buffered.c:1246: tcp_wrbuffer_release(wrb);
net/tcp/tcp_conn.c:741: tcp_wrbuffer_release(wrbuffer);
net/tcp/tcp_conn.c:747: tcp_wrbuffer_release(wrbuffer);
net/tcp/tcp_wrbuffer.c:157: tcp_wrbuffer_release(wrb);
net/tcp/tcp_wrbuffer.c:213: tcp_wrbuffer_release(wrb);
```
like this ?
net/tcp/tcp_wrbuffer.c:
```
tcp_wrbuffer_release(wrb);
#if CONFIG_NET_SEND_BUFSIZE > 0
if (conn)
{
int val = 0;
nxsem_get_value(&conn->snd_sem, &val);
if (val < 0)
{
nxsem_post(&conn->snd_sem);
}
}
#endif
```
net/tcp/tcp_conn.c:
```
tcp_wrbuffer_release(wrb);
#if CONFIG_NET_SEND_BUFSIZE > 0
if (conn)
{
int val = 0;
nxsem_get_value(&conn->snd_sem, &val);
if (val < 0)
{
nxsem_post(&conn->snd_sem);
}
}
#endif
```
--
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] [incubator-nuttx] anchao commented on pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#issuecomment-875244494
> i looked at the tcp part.
>
> * tcp_wrbuffer_tryalloc is not the only place we allocate IOBs. we allocate them in TCP_WBTRYCOPYIN too. i suspect it might be simpler to perform the check in psock_tcp_send.
> * i guess you need to update poll.
@yamt san,
Think for a while, psock_tcp_send not suitable to check the buffer size, because it's hard to find a suitable place to notify that there are available buffers, TCP_WBTRYCOPYIN is a potential risk, but every time of tcp send() will only do the iobcopyin once, the next send() will be check further by tcp_wrbuffer_tryalloc(), I don't think it's a serious scene
--
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] [incubator-nuttx] yamt commented on pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#issuecomment-876807245
> > do you mean where to put the semaphore or an equivalent of it?
> > what's wrong with eg. conn->snd_buf_sem?
> > psock_send_eventhandler can wake it up when some buffers are acked by the peer.
>
> not just psock_send_eventhandler(). I think semaphore put should be done wherever wrbuffer is released.
as far as i understand, the data in the send buffer is disposed only when
* acked by the peer (psock_send_eventhandler)
* connection is closed
>
> ```
> net/tcp/tcp_send_buffered.c:214: tcp_wrbuffer_release((FAR struct tcp_wrbuffer_s *)entry);
> net/tcp/tcp_send_buffered.c:220: tcp_wrbuffer_release((FAR struct tcp_wrbuffer_s *)entry);
> net/tcp/tcp_send_buffered.c:434: tcp_wrbuffer_release(wrb);
> net/tcp/tcp_send_buffered.c:607: tcp_wrbuffer_release(wrb);
> net/tcp/tcp_send_buffered.c:672: tcp_wrbuffer_release(wrb);
> net/tcp/tcp_send_buffered.c:1246: tcp_wrbuffer_release(wrb);
> net/tcp/tcp_conn.c:741: tcp_wrbuffer_release(wrbuffer);
> net/tcp/tcp_conn.c:747: tcp_wrbuffer_release(wrbuffer);
> net/tcp/tcp_wrbuffer.c:157: tcp_wrbuffer_release(wrb);
> net/tcp/tcp_wrbuffer.c:213: tcp_wrbuffer_release(wrb);
> ```
>
> > maybe it works for the majority of cases, yes.
> > i suspect it can be a problem for a small sndbuf though.
>
> This issue is inherent in the configuration of small buffers, SO_SNDBUF is just a suggestion value, How about keeping the current behavior?
i'm not sure what you mean here. can you explain a bit?
--
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] [incubator-nuttx] yamt commented on pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#issuecomment-880405912
> > i'm not sure what you mean here. can you explain a bit?
>
> If we move the check outside the wrbuffer, we will duplicate a lot of sem_post codes in these places. Do you want to do this? I don't think there is any place like net/tcp/tcp_wrbuffer.c is more suitable.
you need to signal waiters for TCP_WBTRIM too.
to me it looks simpler to do it in psock_send_eventhandler directly.
(two places in the function, where `TCP_SEQ_GT(ackno, TCP_WBSEQNO(wrb)` is checked)
>
> ```
> net/tcp/tcp_send_buffered.c:214: tcp_wrbuffer_release((FAR struct tcp_wrbuffer_s *)entry);
> net/tcp/tcp_send_buffered.c:220: tcp_wrbuffer_release((FAR struct tcp_wrbuffer_s *)entry);
> net/tcp/tcp_send_buffered.c:434: tcp_wrbuffer_release(wrb);
> net/tcp/tcp_send_buffered.c:607: tcp_wrbuffer_release(wrb);
> net/tcp/tcp_send_buffered.c:672: tcp_wrbuffer_release(wrb);
> net/tcp/tcp_send_buffered.c:1246: tcp_wrbuffer_release(wrb);
> net/tcp/tcp_conn.c:741: tcp_wrbuffer_release(wrbuffer);
> net/tcp/tcp_conn.c:747: tcp_wrbuffer_release(wrbuffer);
> net/tcp/tcp_wrbuffer.c:157: tcp_wrbuffer_release(wrb);
> net/tcp/tcp_wrbuffer.c:213: tcp_wrbuffer_release(wrb);
> ```
>
> like this ?
yes. but see above.
>
> net/tcp/tcp_wrbuffer.c:
>
> ```
> tcp_wrbuffer_release(wrb);
> #if CONFIG_NET_SEND_BUFSIZE > 0
> if (conn)
> {
> int val = 0;
>
> nxsem_get_value(&conn->snd_sem, &val);
> if (val < 0)
> {
> nxsem_post(&conn->snd_sem);
> }
> }
> #endif
> ```
>
> net/tcp/tcp_conn.c:
>
> ```
> tcp_wrbuffer_release(wrb);
> #if CONFIG_NET_SEND_BUFSIZE > 0
> if (conn)
> {
> int val = 0;
>
> nxsem_get_value(&conn->snd_sem, &val);
> if (val < 0)
> {
> nxsem_post(&conn->snd_sem);
> }
> }
> #endif
> ```
--
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] [incubator-nuttx] xiaoxiang781216 commented on pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#issuecomment-879891624
Let's add the wait in psock_tcp_send and notify in the done callback directly?
--
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] [incubator-nuttx] xiaoxiang781216 merged pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080
--
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] [incubator-nuttx] yamt commented on pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#issuecomment-875255375
> > i looked at the tcp part.
> >
> > * tcp_wrbuffer_tryalloc is not the only place we allocate IOBs. we allocate them in TCP_WBTRYCOPYIN too. i suspect it might be simpler to perform the check in psock_tcp_send.
> > * i guess you need to update poll.
>
> @yamt san,
> Think for a while, psock_tcp_send not suitable to check the buffer size, because it's hard to find a suitable place to notify that there are available buffers,
do you mean where to put the semaphore or an equivalent of it?
what's wrong with eg. conn->snd_buf_sem?
psock_send_eventhandler can wake it up when some buffers are acked by the peer.
> TCP_WBTRYCOPYIN is a potential risk, but every time of tcp send() will only do the iobcopyin once, the next send() will be check further by tcp_wrbuffer_tryalloc(), I don't think it's a serious scene
maybe it works for the majority of cases, yes.
i suspect it can be a problem for a small sndbuf though.
--
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] [incubator-nuttx] anchao commented on pull request #4080: net/socket: add SO_SNDBUF support
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #4080:
URL: https://github.com/apache/incubator-nuttx/pull/4080#issuecomment-875272845
> do you mean where to put the semaphore or an equivalent of it?
> what's wrong with eg. conn->snd_buf_sem?
> psock_send_eventhandler can wake it up when some buffers are acked by the peer.
not just psock_send_eventhandler(). I think semaphore put should be done wherever wrbuffer is released.
```
net/tcp/tcp_send_buffered.c:214: tcp_wrbuffer_release((FAR struct tcp_wrbuffer_s *)entry);
net/tcp/tcp_send_buffered.c:220: tcp_wrbuffer_release((FAR struct tcp_wrbuffer_s *)entry);
net/tcp/tcp_send_buffered.c:434: tcp_wrbuffer_release(wrb);
net/tcp/tcp_send_buffered.c:607: tcp_wrbuffer_release(wrb);
net/tcp/tcp_send_buffered.c:672: tcp_wrbuffer_release(wrb);
net/tcp/tcp_send_buffered.c:1246: tcp_wrbuffer_release(wrb);
net/tcp/tcp_conn.c:741: tcp_wrbuffer_release(wrbuffer);
net/tcp/tcp_conn.c:747: tcp_wrbuffer_release(wrbuffer);
net/tcp/tcp_wrbuffer.c:157: tcp_wrbuffer_release(wrb);
net/tcp/tcp_wrbuffer.c:213: tcp_wrbuffer_release(wrb);
```
> maybe it works for the majority of cases, yes.
> i suspect it can be a problem for a small sndbuf though.
This issue is inherent in the configuration of small buffers, SO_SNDBUF is just a suggestion value, How about keeping the current behavior?
--
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