[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3081: tcp_send_buffered.c: improve tcp write buffering

[GitBox <gi...@apache.org>]

yamt commented on a change in pull request #3081:
URL: https://github.com/apache/incubator-nuttx/pull/3081#discussion_r598356547



##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1093,11 +1114,63 @@ 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;
 
+      /* Allocate a write buffer.  Careful, the network will be momentarily
+       * unlocked here.
+       */
+
+      /* Try to coalesce into the last wrb.
+       *
+       * But only when it might yield larger segments.
+       * (REVISIT: It might make sense to lift this condition.
+       * IOB boundaries and segment boundaries usually do not match.
+       * It makes sense to save the number of IOBs.)
+       *
+       * Also, for simplicity, do it only when we haven't sent anything
+       * from the the wrb yet.
+       */
+
+      const uint32_t max_wrb_size = tcp_max_wrb_size(conn);

Review comment:
       > Please move the variable declaration to the beginning of the function!
   
   fixed

##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1093,11 +1114,63 @@ 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;
 
+      /* Allocate a write buffer.  Careful, the network will be momentarily
+       * unlocked here.
+       */
+
+      /* Try to coalesce into the last wrb.
+       *
+       * But only when it might yield larger segments.
+       * (REVISIT: It might make sense to lift this condition.
+       * IOB boundaries and segment boundaries usually do not match.
+       * It makes sense to save the number of IOBs.)
+       *
+       * Also, for simplicity, do it only when we haven't sent anything
+       * from the the wrb yet.
+       */
+
+      const uint32_t max_wrb_size = tcp_max_wrb_size(conn);
+      wrb = (FAR struct tcp_wrbuffer_s *)sq_tail(&conn->write_q);
+      if (wrb != NULL && TCP_WBSENT(wrb) == 0 && TCP_WBNRTX(wrb) == 0 &&
+          TCP_WBPKTLEN(wrb) < max_wrb_size &&
+          (TCP_WBPKTLEN(wrb) % conn->mss) != 0)
+        {
+          wrb = (FAR struct tcp_wrbuffer_s *)sq_remlast(&conn->write_q);
+          ninfo("coalesce %zu bytes to wrb %p (%" PRIu16 ")\n", len, wrb,
+                TCP_WBPKTLEN(wrb));
+          DEBUGASSERT(TCP_WBPKTLEN(wrb) > 0);
+        }
+      else if (nonblock)
+        {
+          wrb = tcp_wrbuffer_tryalloc();
+          ninfo("new wrb %p (non blocking)\n", wrb);
+        }
+      else
+        {
+          wrb = tcp_wrbuffer_alloc();
+          ninfo("new wrb %p\n", wrb);
+        }
+
+      if (wrb == NULL)
+        {
+          /* A buffer allocation error occurred */
+
+          nerr("ERROR: Failed to allocate write buffer\n");
+          ret = nonblock ? -EAGAIN : -ENOMEM;
+          goto errout_with_lock;
+        }
+
       /* Initialize the write buffer */
 
       TCP_WBSEQNO(wrb) = (unsigned)-1;
       TCP_WBNRTX(wrb)  = 0;
 
+      unsigned int off = TCP_WBPKTLEN(wrb);

Review comment:
       fixed




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org