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/06/05 11:38:31 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request, #6368: net/tcp: Hold the net lock in tcp_timer_expiry
xiaoxiang781216 opened a new pull request, #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368
## Summary
to follow the call convention for d_txavail
## Impact
tcp timer process
## Testing
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r889861137
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
This is just another work queue, why not directly use work_s directly like #6355? The additional cost is very small since network stack already depend on the system work.
--
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 #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#issuecomment-1146835311
> @xiaoxiang781216 could you please remove all the `devif_timer` from the comments all over the code since it sipped from the #6355 changes? Or a separate PR is needed for that?
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] masayuki2009 commented on pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#issuecomment-1148085128
@xiaoxiang781216
The ping command still does not work with spresense:rndis.
```
NuttShell (NSH) NuttX-3.6.1
nsh> uname -a
NuttX 3.6.1 c4e78abb42 Jun 7 2022 10:00:58 arm spresense
nsh> ps
PID GROUP PRI POLICY TYPE NPX STATE EVENT SIGMASK STACK USED FILLED COMMAND
0 0 0 FIFO Kthread N-- Ready 00000000 001000 000372 37.2% Idle Task
1 1 224 RR Kthread --- Waiting Signal 00000000 001992 000460 23.0% hpwork 0x2d0502e8
2 2 100 RR Kthread --- Waiting Semaphore 00000000 001992 000276 13.8% lpwork 0x2d0502f8
3 3 100 RR Task --- Running 00000000 003024 001224 40.4% spresense_main
4 4 200 RR Task --- Waiting MQ empty 00000000 000976 000440 45.0% cxd56_pm_task
5 5 100 RR Task --- Waiting Semaphore 00000000 001984 000516 26.0% Telnet daemon 0x2d065940
nsh> free
total used free largest nused nfree
Umem: 1200720 49472 1151248 1150112 137 2
nsh> ifconfig
lo Link encap:Local Loopback at RUNNING
inet addr:127.0.0.1 DRaddr:127.0.0.1 Mask:255.0.0.0
eth0 Link encap:Ethernet HWaddr 00:e0:de:ad:be:ff at UP
inet addr:10.0.0.2 DRaddr:10.0.0.1 Mask:255.255.255.0
IPv4 TCP UDP ICMP
Received 0000 0000 0000 0000
Dropped 0000 0000 0000 0000
IPv4 VHL: 0000 Frg: 0000
Checksum 0000 0000 0000 ----
TCP ACK: 0000 SYN: 0000
RST: 0000 0000
Type 0000 ---- ---- 0000
Sent 0000 0000 0000 0000
Rexmit ---- 0000 ---- ----
nsh> ifconfig eth0 10.0.1.20
nsh> ifconfig
lo Link encap:Local Loopback at RUNNING
inet addr:127.0.0.1 DRaddr:127.0.0.1 Mask:255.0.0.0
eth0 Link encap:Ethernet HWaddr 00:e0:de:ad:be:ff at UP
inet addr:10.0.1.20 DRaddr:10.0.1.1 Mask:255.255.255.0
IPv4 TCP UDP ICMP
Received 0000 0000 0000 0000
Dropped 0000 0000 0000 0000
IPv4 VHL: 0000 Frg: 0000
Checksum 0000 0000 0000 ----
TCP ACK: 0000 SYN: 0000
RST: 0000 0000
Type 0000 ---- ---- 0000
Sent 0000 0000 0000 0000
Rexmit ---- 0000 ---- ----
nsh> ping 10.0.1.20
PING 10.0.1.20 56 bytes of data
```
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r889705947
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
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] xiaoxiang781216 commented on a diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r889705947
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
Done, but I am a little bit concern the performance.
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r889702419
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
In general looks good, but I have a question. Is there a room for a race condition if `tcp_timer_expiry` is preempted by a thread running dev_poll so after returning back `arg` will no longer point to a valid `struct tcp_conn_s` structure? I mean maybe it is better to iterate
```
FAR struct tcp_conn_s *conn;
net_lock();
while ((conn = tcp_nextconn(conn)) != NULL && conn != arg);
if (conn != NULL)
{
conn->timeout = true;
netdev_txnotify_dev(conn->dev);
}
net_unlock();
```
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r890124466
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
> This is just another work queue, why not directly use work_s directly like #6355? The additional cost is very small since network stack already depend on the system work.
Could you please expand the description. I'm not fully tracking what you are trying to say.
I see that `struct tcp_conn_s` was equipped with `struct work_s work; /* TCP timer handle */` in https://github.com/apache/incubator-nuttx/pull/6330 so we start getting work queue per TCP connecting instead of a single work queue polling `devif_timer`. Then a polling work queue has been removed by https://github.com/apache/incubator-nuttx/pull/6355. Is that correct?
If above is "yes", then why we can't remove `struct work_s` from `struct tcp_conn_s` and adding a global `struct work_s tcp_timer_work;`. Each `truct tcp_conn_s` will have `int time_left;` field. The `tcp_timer_work` will `tcp_nextconn(conn)` and do `conn->time_left -= delay;` and `conn->timeout = true;` if delay is `0`. The `tcp_timer_work` can be started passing the delay via `arg`, so each `tcp_update_timer` call can check and adjust `tcp_timer_work` is connection timeout is less than time left since work queue is scheduled.
Maybe I'm missing some points, so please help me to understand.
--
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 pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#issuecomment-1146819573
@xiaoxiang781216 could you please remove all the `devif_timer` from the comments all over the code since it sipped from the https://github.com/apache/incubator-nuttx/pull/6355 changes? Or a separate PR is needed for that?
--
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 merged pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r889787181
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
We are developing a wearable device need work for two weeks with 450mhA, so it isn't good to check the timeout every half second. That's why @zhhyu7 create https://github.com/apache/incubator-nuttx/pull/6355 in the first place.
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r890135645
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
> > This is just another work queue, why not directly use work_s directly like #6355? The additional cost is very small since network stack already depend on the system work.
>
> Could you please expand the description. I'm not fully tracking what you are trying to say. I see that `struct tcp_conn_s` was equipped with `struct work_s work; /* TCP timer handle */` in #6330 so we start getting work queue per TCP connecting instead of a single work queue polling `devif_timer`. Then a polling work queue has been removed by #6355. Is that correct?
>
Yes, but the term usage isn't accurate. Both still use the same(LPWORK) work queue. The difference is that the old use one work_s for each netdev driver, but the new use one work_s for each tcp conn.
Note: work_s isn't work queue, which is a simple struct(contain timeout, callback etc), not 1:1 mapping to thread.
> If above is "yes", then why we can't remove `struct work_s` from `struct tcp_conn_s` and adding a global `struct work_s tcp_timer_work;`. Each `truct tcp_conn_s` will have `int time_left;` field. The `tcp_timer_work` will `tcp_nextconn(conn)` and do `conn->time_left -= delay;` and `conn->timeout = true;` if delay is `0`. The `tcp_timer_work` can be started passing the delay via `arg`, so each `tcp_update_timer` call can check and adjust `tcp_timer_work` is connection timeout is less than time left since work queue is scheduled.
>
This is just how the work queue work:
https://github.com/apache/incubator-nuttx/blob/master/sched/wqueue/kwork_thread.c#L156-L180
> Maybe I'm missing some points, so please help me to understand.
My point is that we can reuse sched/wqueue code, there isn't any benefit to write a dedicated work queue for network stack.
--
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 #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#issuecomment-1148186911
Sure, no problem.
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r890160023
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
Since the timeout only happen in the abnormal condition, the performance lose should be small.
All code which use wqueue suffer the same race condition, even the old netdev's work_s since netdev may unregister or stop while the work is pending to run. The solution is implemented the sync cancel like how Linux do:
https://www.kernel.org/doc/html/v4.13/driver-api/basics.html#c.cancel_delayed_work
https://www.kernel.org/doc/html/v4.13/driver-api/basics.html#c.cancel_delayed_work_sync
Basically, the key issue is that NuttX work queue lack the sync version for cancel.
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r890160023
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
Since the timeout only happen in the abnormal condition, the performance lose should be small.
All code which use wqueue suffer the same race condition, even the old netdev's work_s since netdev may unregister or stop while the work is pending to run. The solution is implemented the sync cancel like how Linux do:
https://www.kernel.org/doc/html/v4.13/driver-api/basics.html#c.cancel_delayed_work
https://www.kernel.org/doc/html/v4.13/driver-api/basics.html#c.cancel_delayed_work_sync
Basically, NuttX work queue lack the sync version of cacnel.
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r889819468
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
Performance value is zero if the system is multi threading unsafe. Let's merge current change and think about performance improvement. Maybe having a single work queue that is running at minimum of all connections timeout and iterating all the connections may be a solution instead of half a second periodic execution. I mean that it is a classic tick vs tickles mode problem, so we can apply a classic solution for it
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r889862388
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
Let me analyze a bit more
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r890135645
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
> > This is just another work queue, why not directly use work_s directly like #6355? The additional cost is very small since network stack already depend on the system work.
>
> Could you please expand the description. I'm not fully tracking what you are trying to say. I see that `struct tcp_conn_s` was equipped with `struct work_s work; /* TCP timer handle */` in #6330 so we start getting work queue per TCP connecting instead of a single work queue polling `devif_timer`. Then a polling work queue has been removed by #6355. Is that correct?
>
Yes, but the term usage isn't accurate. Both still use the same(LPWORK) work queue. The difference is that the old use one work_s for each netdev driver, but the new use one work_s for each tcp conn.
Note: work_s isn't work queue(thread), but a simple struct(contain timeout, callback etc).
> If above is "yes", then why we can't remove `struct work_s` from `struct tcp_conn_s` and adding a global `struct work_s tcp_timer_work;`. Each `truct tcp_conn_s` will have `int time_left;` field. The `tcp_timer_work` will `tcp_nextconn(conn)` and do `conn->time_left -= delay;` and `conn->timeout = true;` if delay is `0`. The `tcp_timer_work` can be started passing the delay via `arg`, so each `tcp_update_timer` call can check and adjust `tcp_timer_work` is connection timeout is less than time left since work queue is scheduled.
>
This is just how the work queue work:
https://github.com/apache/incubator-nuttx/blob/master/sched/wqueue/kwork_thread.c#L156-L180
> Maybe I'm missing some points, so please help me to understand.
My point is that we can reuse sched/wqueue code, there isn't any benefit to write a dedicated work queue for network stack.
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r890177392
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
Ok. So let's keep the current version.
--
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] masayuki2009 commented on pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#issuecomment-1148140279
>@xiaoxiang781216
>
>The ping command still does not work with spresense:rndis.
>The issue happened with https://github.com/apache/incubator-nuttx/pull/6355
I found that the issue does not relate to the #6355 but relates to my test case.
Sorry for the confusion.
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r891508282
##########
net/arp/arp_poll.c:
##########
@@ -45,8 +45,8 @@
*
* Assumptions:
* This function is called from the MAC device driver indirectly through
- * devif_poll() and devif_timer() and may be called from the timer
- * interrupt/watchdog handler level.
+ * devif_poll() and may be called from the timer interrupt/watchdog
Review Comment:
```suggestion
* devif_poll() and may be called from the timer interrupt/watchdog
```
--
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r889727647
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
Then maybe it is better to get rid of work queue per connection and have a single tcp_timer work queue that will run periodically with half second resolution and will integrate all the TCP connections and decrement timeout counters? What 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] xiaoxiang781216 commented on pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#issuecomment-1146835342
> @xiaoxiang781216 could you please remove all the `devif_timer` from the comments all over the code since it sipped from the #6355 changes? Or a separate PR is needed for that?
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 diff in pull request #6368: net/tcp: Hold the net lock in tcp_timer_expiry
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6368:
URL: https://github.com/apache/incubator-nuttx/pull/6368#discussion_r890150666
##########
net/tcp/tcp_timer.c:
##########
@@ -139,8 +140,10 @@ static void tcp_timer_expiry(FAR void *arg)
{
FAR struct tcp_conn_s *conn = arg;
+ net_lock();
Review Comment:
No. I'm not talking about separate sched/wqueue for network stack, but reusing an LPWORK for that, but adding a single TCP work function to LPWORK queue that will iterate all the connections with lock taken. I do not see how we have achieve multi thread safety and good performance in other way.
```
leave_critical_section(flags);
CALL_WORKER(worker, arg);
flags = enter_critical_section();
```
The preemption may happen right after `leave_critical_section(flags);` and before `CALL_WORKER(worker, arg);`, so do not see how we can rely on `arg` in `tcp_timer_expiry` without a `tcp_nextconn()` loop. But that seems to give performance penalty, so we need to conduct a more lightweight solution.
--
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