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 2022/02/17 06:36:34 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #5526: net/tcp: add support for send timeout on buffer mode
anchao opened a new pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526
## Summary
net/tcp: add support for send timeout on buffer mode
Signed-off-by: chao.an <an...@xiaomi.com>
## Impact
N/A
## Testing
iperf test
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809700327
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1210,11 +1247,11 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
{
wrb = tcp_wrbuffer_tryalloc();
ninfo("new wrb %p (non blocking)\n", wrb);
- DEBUGASSERT(wrb == NULL || TCP_WBPKTLEN(wrb) == 0);
}
else
{
- wrb = tcp_wrbuffer_alloc();
+ wrb = tcp_wrbuffer_timedalloc(tcp_send_gettimeout(start,
+ timeout));
ninfo("new wrb %p\n", wrb);
DEBUGASSERT(TCP_WBPKTLEN(wrb) == 0);
Review comment:
should we remove DEBUGASSERT?
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808875154
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1108,6 +1117,15 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
nonblock = _SS_ISNONBLOCK(conn->sconn.s_flags) ||
(flags & MSG_DONTWAIT) != 0;
+ expire = _SO_TIMEOUT(conn->sconn.s_sndtimeo);
+ if (expire != UINT_MAX)
+ {
+ expire += TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ expire--;
Review comment:
`clock_systime_ticks` is either 32 or 64 bits and clock is continuously incremented so sequential `TICK2MSEC(clock_systime_ticks())` can be `TICK2MSEC(0xFFFFFFFF)` -> `TICK2MSEC(1)`.
For example `_SO_TIMEOUT(conn->sconn.s_sndtimeo)` is `UINT_MAX - 5`, then we call `expire += TICK2MSEC(0xFFFFFFFF);` and that will most probably overflow and then we check `if (expire == UINT_MAX)` that is not hit.
Please describe more the boundary use cases so we can be sure that timeout logic is bulletproof.
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808864088
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1108,6 +1117,15 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
nonblock = _SS_ISNONBLOCK(conn->sconn.s_flags) ||
(flags & MSG_DONTWAIT) != 0;
+ expire = _SO_TIMEOUT(conn->sconn.s_sndtimeo);
+ if (expire != UINT_MAX)
+ {
+ expire += TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ expire--;
Review comment:
to avoid overflow
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809079041
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
> I can keep the anchor prototype as clock_t, but the remain still needs.
> remain timeout parameter needs to be calculated dynamically, this time needs to be accumulated
>
> ```
> 1207 remain = tcp_send_gettimeout(anchor, remain);
> 1208 ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
> //remain changed
> 1253 remain = tcp_send_gettimeout(anchor, remain);
> 1254 wrb = tcp_wrbuffer_timedalloc(remain);
> 1255 ninfo("new wrb %p\n", wrb);
> 1256 DEBUGASSERT(TCP_WBPKTLEN(wrb) == 0);
> ...
> ```
Please examine my suggestion again. It calculates parameter dynamically
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809072409
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
I feel weird that my original commit was using clock_t, I change the clock_t to uint32_t cause of your comment
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808809882
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1108,6 +1117,15 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
nonblock = _SS_ISNONBLOCK(conn->sconn.s_flags) ||
(flags & MSG_DONTWAIT) != 0;
+ expire = _SO_TIMEOUT(conn->sconn.s_sndtimeo);
+ if (expire != UINT_MAX)
+ {
+ expire += TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ expire--;
Review comment:
why `--` and not `++`?
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1190,25 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ current = TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ remain = UINT_MAX;
+ }
+ else if (expire > current)
+ {
+ remain = expire - current;
+ }
+ else
+ {
+ remain = 0;
+ }
+
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
shouldn't this be wrapped with `if (remain > 0)`?
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1190,25 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ current = TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ remain = UINT_MAX;
+ }
+ else if (expire > current)
+ {
+ remain = expire - current;
+ }
+ else
+ {
+ remain = 0;
Review comment:
Shouldn't we have here
```suggestion
remain = current - expire;
```
?
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1053,6 +1053,15 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
ssize_t result = 0;
bool nonblock;
int ret = OK;
+#ifdef CONFIG_HAVE_LONG_LONG
+ uint64_t expire;
+ uint64_t remain;
+ uint64_t current;
+#else
+ clock_t expire;
+ clock_t remain;
+ clock_t current;
+#endif
Review comment:
```suggestion
unsigned int expire;
unsigned int remain;
unsigned int current;
```
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808877388
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1190,25 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ current = TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ remain = UINT_MAX;
+ }
+ else if (expire > current)
+ {
+ remain = expire - current;
+ }
+ else
+ {
+ remain = 0;
Review comment:
```
static int
_net_timedwait(sem_t *sem, bool interruptible, unsigned int timeout)
{
unsigned int count;
irqstate_t flags;
int blresult;
int ret;
flags = enter_critical_section(); /* No interrupts */
sched_lock(); /* No context switches */
/* Release the network lock, remembering my count. net_breaklock will
* return a negated value if the caller does not hold the network lock.
*/
blresult = net_breaklock(&count);
/* Now take the semaphore, waiting if so requested. */
if (timeout != UINT_MAX)
{
struct timespec abstime;
DEBUGVERIFY(clock_gettime(CLOCK_REALTIME, &abstime));
abstime.tv_sec += timeout / MSEC_PER_SEC;
abstime.tv_nsec += timeout % MSEC_PER_SEC * NSEC_PER_MSEC;
if (abstime.tv_nsec >= NSEC_PER_SEC)
{
abstime.tv_sec++;
abstime.tv_nsec -= NSEC_PER_SEC;
}
/* Wait until we get the lock or until the timeout expires */
if (interruptible)
{
ret = nxsem_timedwait(sem, &abstime);
}
else
{
ret = nxsem_timedwait_uninterruptible(sem, &abstime);
}
}
else
{
/* Wait as long as necessary to get the lock */
if (interruptible)
{
ret = nxsem_wait(sem);
}
else
{
ret = nxsem_wait_uninterruptible(sem);
}
}
/* Recover the network lock at the proper count (if we held it before) */
if (blresult >= 0)
{
net_restorelock(count);
}
sched_unlock();
leave_critical_section(flags);
return ret;
}
```
No trywait in case if `timeout == 0`.
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#issuecomment-1042621242
Depends on: https://github.com/apache/incubator-nuttx/pull/5524 https://github.com/apache/incubator-nuttx/pull/5525
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809069471
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
I can keep the anchor prototype as clock_t, but the remain still needs.
remain timeout parameter needs to be calculated dynamically, this time needs to be accumulated
```
1207 remain = tcp_send_gettimeout(anchor, remain);
1208 ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
//remain changed
1253 remain = tcp_send_gettimeout(anchor, remain);
1254 wrb = tcp_wrbuffer_timedalloc(remain);
1255 ninfo("new wrb %p\n", wrb);
1256 DEBUGASSERT(TCP_WBPKTLEN(wrb) == 0);
...
```
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809012387
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
Is it possible to go with
```
clock_t start;
...
start = clock_systime_ticks();
...
ret = net_timedwait_uninterruptible(&conn->snd_sem, tcp_send_gettimeout(start, _SO_TIMEOUT(conn->sconn.s_sndtimeo)));
```
and modify `tcp_send_gettimeout` to
```
static unsigned int tcp_send_gettimeout(clock_t start, unsigned int timeout)
{
clock_t elapse;
if (remain != UINT_MAX)
{
elapse = TICK2MSEC(clock_systime_ticks() - start);
if (elapse >= remain)
{
remain = 0;
}
else
{
remain -= elapse;
}
}
return remain;
}
```
?
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808991846
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1177,24 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ if (remain != UINT_MAX)
Review comment:
this code is repeated 3 times. Maybe we can build a small support function and use it? Something like:
```
uint32_t tcp_send_gettimeout(uint32_t start, uint32_t remain)
{
uint32_t elapse;
if (remain != UINT_MAX)
{
elapse = TICK2MSEC(clock_systime_ticks()) - start;
if (elapse >= remain)
{
remain = 0;
}
else
{
remain -= elapse;
}
}
return remain;
}
```
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809012387
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
Is it possible to go with
```
clock_t start;
...
start = clock_systime_ticks();
...
ret = net_timedwait_uninterruptible(&conn->snd_sem, tcp_send_gettimeout(start, _SO_TIMEOUT(conn->sconn.s_sndtimeo)));
```
and modify `tcp_send_gettimeout` to
```
static unsigned int tcp_send_gettimeout(clock_t start, unsigned int timeout)
{
unsigned int elapsed;
if (timeout != UINT_MAX)
{
elapsed = TICK2MSEC(clock_systime_ticks() - start);
if (elapsed >= timeout)
{
timeout = 0;
}
else
{
timeout -= elapsed;
}
}
return timeout;
}
```
?
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809076452
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
The type depends either we do value saving is unconverted or converted with `TICK2MSEC`
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808877388
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1190,25 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ current = TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ remain = UINT_MAX;
+ }
+ else if (expire > current)
+ {
+ remain = expire - current;
+ }
+ else
+ {
+ remain = 0;
Review comment:
```
static int
_net_timedwait(sem_t *sem, bool interruptible, unsigned int timeout)
{
unsigned int count;
irqstate_t flags;
int blresult;
int ret;
flags = enter_critical_section(); /* No interrupts */
sched_lock(); /* No context switches */
/* Release the network lock, remembering my count. net_breaklock will
* return a negated value if the caller does not hold the network lock.
*/
blresult = net_breaklock(&count);
/* Now take the semaphore, waiting if so requested. */
if (timeout != UINT_MAX)
{
struct timespec abstime;
DEBUGVERIFY(clock_gettime(CLOCK_REALTIME, &abstime));
abstime.tv_sec += timeout / MSEC_PER_SEC;
abstime.tv_nsec += timeout % MSEC_PER_SEC * NSEC_PER_MSEC;
if (abstime.tv_nsec >= NSEC_PER_SEC)
{
abstime.tv_sec++;
abstime.tv_nsec -= NSEC_PER_SEC;
}
/* Wait until we get the lock or until the timeout expires */
if (interruptible)
{
ret = nxsem_timedwait(sem, &abstime);
}
else
{
ret = nxsem_timedwait_uninterruptible(sem, &abstime);
}
}
else
{
/* Wait as long as necessary to get the lock */
if (interruptible)
{
ret = nxsem_wait(sem);
}
else
{
ret = nxsem_wait_uninterruptible(sem);
}
}
/* Recover the network lock at the proper count (if we held it before) */
if (blresult >= 0)
{
net_restorelock(count);
}
sched_unlock();
leave_critical_section(flags);
return ret;
}
```
No `nxsem_trywait` in case if `timeout == 0`.
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809012387
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
Is it possible to go with
```
clock_t start;
...
start = clock_systime_ticks();
...
ret = net_timedwait_uninterruptible(&conn->snd_sem, tcp_send_gettimeout(start, _SO_TIMEOUT(conn->sconn.s_sndtimeo)));
```
and modify `tcp_send_gettimeout` to
```
static unsigned int tcp_send_gettimeout(clock_t start, unsigned int timeout)
{
unsigned int elapse;
if (remain != UINT_MAX)
{
elapse = TICK2MSEC(clock_systime_ticks() - start);
if (elapse >= remain)
{
remain = 0;
}
else
{
remain -= elapse;
}
}
return remain;
}
```
?
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808863477
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1053,6 +1053,15 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
ssize_t result = 0;
bool nonblock;
int ret = OK;
+#ifdef CONFIG_HAVE_LONG_LONG
+ uint64_t expire;
+ uint64_t remain;
+ uint64_t current;
+#else
+ clock_t expire;
+ clock_t remain;
+ clock_t current;
+#endif
Review comment:
expire is absolute time in milliseconds
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808883076
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1190,25 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ current = TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ remain = UINT_MAX;
+ }
+ else if (expire > current)
+ {
+ remain = expire - current;
+ }
+ else
+ {
+ remain = 0;
Review comment:
trywait:
https://github.com/apache/incubator-nuttx/blob/master/sched/semaphore/sem_clockwait.c#L125
timeout:
https://github.com/apache/incubator-nuttx/blob/master/sched/semaphore/sem_clockwait.c#L156
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808889807
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1190,25 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ current = TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ remain = UINT_MAX;
+ }
+ else if (expire > current)
+ {
+ remain = expire - current;
+ }
+ else
+ {
+ remain = 0;
Review comment:
Thanks!
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808866316
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1053,6 +1053,15 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
ssize_t result = 0;
bool nonblock;
int ret = OK;
+#ifdef CONFIG_HAVE_LONG_LONG
+ uint64_t expire;
+ uint64_t remain;
+ uint64_t current;
+#else
+ clock_t expire;
+ clock_t remain;
+ clock_t current;
+#endif
Review comment:
Yes. I understand this, but since it is compared against `UINT_MAX` I do not think that there is a need for `uint64_t`. Or I'm missing something?
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809031539
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
1. anchor time should not to be changed on sending process,
2. remain, anchor, _SO_TIMEOUT(conn->sconn.s_sndtimeo), these are in milliseconds, which will reduce unnecessary type cast
3. what different with current tcp_send_gettimeout ?
--
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809052145
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
> 1. anchor time should not to be changed on sending process,
>
I do not propose to change anchor time on sending process. It will be done once. I just used a different name `anchor` -> `start`.
> 2. remain, anchor, _SO_TIMEOUT(conn->sconn.s_sndtimeo), these are in milliseconds, which will reduce unnecessary type cast
>
I do not fully understand this. In my proposal I don't think that there are additional type casts.
> 3. what different with current tcp_send_gettimeout ?
In general I propose to track running time in `clock_t` (the same type that is returned by `clock_systime_ticks`) and perform conversion to `ms` only inside the `tcp_send_gettimeout` once. This will give better granularity compared to current code. Also it will eliminate having `uint32_t remain;` inside the `psock_tcp_send`.
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809097536
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
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 a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808985350
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1053,6 +1053,15 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
ssize_t result = 0;
bool nonblock;
int ret = OK;
+#ifdef CONFIG_HAVE_LONG_LONG
+ uint64_t expire;
+ uint64_t remain;
+ uint64_t current;
+#else
+ clock_t expire;
+ clock_t remain;
+ clock_t current;
+#endif
Review comment:
Done
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1108,6 +1117,15 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
nonblock = _SS_ISNONBLOCK(conn->sconn.s_flags) ||
(flags & MSG_DONTWAIT) != 0;
+ expire = _SO_TIMEOUT(conn->sconn.s_sndtimeo);
+ if (expire != UINT_MAX)
+ {
+ expire += TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ expire--;
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] pkarashchenko commented on a change in pull request #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r809012387
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1204,12 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ remain = tcp_send_gettimeout(anchor, remain);
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
Is it possible to go with
```
clock_t start;
...
start = clock_systime_ticks();
...
ret = net_timedwait_uninterruptible(&conn->snd_sem, tcp_send_gettimeout(start, _SO_TIMEOUT(conn->sconn.s_sndtimeo)));
```
and modify `tcp_send_gettimeout` to
```
static unsigned int tcp_send_gettimeout(clock_t start, unsigned int timeout)
{
unsigned int elapsed;
if (remain != UINT_MAX)
{
elapsed = TICK2MSEC(clock_systime_ticks() - start);
if (elapsed >= remain)
{
remain = 0;
}
else
{
remain -= elapsed;
}
}
return remain;
}
```
?
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808867410
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1190,25 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ current = TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ remain = UINT_MAX;
+ }
+ else if (expire > current)
+ {
+ remain = expire - current;
+ }
+ else
+ {
+ remain = 0;
Review comment:
```
if (expire == UINT_MAX)
{
remain = UINT_MAX; <--- wait
}
else if (expire > current)
{
remain = expire - current; <--- timed wait
}
else
{
remain = 0; <----- trywait
}
```
remain in 0 is meaningful and is used to identify try wait
--
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 #5526: net/tcp: add support for send timeout on buffer mode
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5526:
URL: https://github.com/apache/incubator-nuttx/pull/5526#discussion_r808869846
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1172,7 +1190,25 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
goto errout_with_lock;
}
- net_lockedwait_uninterruptible(&conn->snd_sem);
+ current = TICK2MSEC(clock_systime_ticks());
+ if (expire == UINT_MAX)
+ {
+ remain = UINT_MAX;
+ }
+ else if (expire > current)
+ {
+ remain = expire - current;
+ }
+ else
+ {
+ remain = 0;
+ }
+
+ ret = net_timedwait_uninterruptible(&conn->snd_sem, remain);
Review comment:
which is impossible,
```
current = TICK2MSEC(clock_systime_ticks());
if (expire == UINT_MAX)
{
remain = UINT_MAX; = UINT_MAX
}
else if (expire > current)
{
remain = expire - current; > 0
}
else
{
remain = 0; = 0
}
```
--
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