drivers/serial/serial.c: adapt to the iovec-based api #14898

yamt · 2024-11-22T02:43:39Z

Summary

drivers/serial/serial.c: adapt to the iovec-based api

This would fix readv/writev issues mentioned in
#12674.
(only for this specific driver though. with this approach,
we basically have to fix every single drivers and
filesystems.)

Impact

Testing

Lightly tested on the serial console, using micropython REPL
on toywasm with esp32s3-devkit:toywasm, which used to be
suffered by the readv issue.

nuttxpr · 2024-11-22T02:50:04Z

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

No, this PR does not fully meet the NuttX requirements. While it provides a summary and mentions testing, it lacks crucial details.

Here's a breakdown of missing information:

Summary: While the "what" is explained, the "how" is missing. The summary should explain precisely how serial.c is adapted to the iovec-based API. What code changes were made? What functions were modified?
Impact: This section is entirely empty. All impact points need to be addressed, even if the answer is "NO". For example, if no user impact is expected, explicitly state "Impact on user: NO". The developer needs to consider and document every potential impact. Given that this PR mentions fixing a bug, there's likely impact on compatibility (fixing a bug improves compatibility, arguably).
Testing: The testing description is insufficient. "Lightly tested" is vague. What specific tests were run? What commands were issued? The provided information about the platform is good, but the actual logs are missing. Empty code blocks are provided for "Testing logs before change" and "Testing logs after change" - these must be populated with actual output.

Specifically, the author needs to:

Elaborate on the implementation details in the summary.
Complete the Impact section, addressing each point individually.
Provide concrete testing details and include the actual logs before and after the change.

include/nuttx/fs/uio.h

xiaoxiang781216 · 2024-11-23T05:19:24Z

fs/vfs/fs_read.c

@@ -102,6 +101,11 @@ static ssize_t file_readv_compat(FAR struct file *filep,
      remaining -= nread;
    }

+  if (ntotal >= 0)
+    {
+      uio_advance(uio, ntotal);


what's benefit to update the offset in one of uio entry

it eases logic like:

while (uio_resid(uio) > 0) { n = process_some(uio, ...); uio_advance(uio, n); }

and maybe can help things like restart on EINTR in future.

can we advance in the loop to avoid iterate the vector twice

yes, we can. i feel it's one of the last things to optimize though.

xiaoxiang781216 · 2024-11-23T05:21:36Z

fs/vfs/fs_uio.c

 *   Or -EOVERFLOW.
 *
 ****************************************************************************/

-ssize_t uio_total_len(FAR const struct uio *uio)
+ssize_t uio_resid(FAR const struct uio *uio)


uio_remain?

"resid" is the term used by BSDs.

Ok, can we follow https://man.freebsd.org/cgi/man.cgi?uio(9) as much as possible, which define uio_offset and uio_resid?

ok. let me think i bit.

fs/vfs/fs_uio.c

This can simplify partial result handling.

"total" is a bit confusing after the introduction of uio_advance().

although i'm not quite happy with the "offset" functionality, it's necessary to simplify the adaptation of some drivers like drivers/serial/serial.c, which (ab)uses the user-supplied buffer as a line-buffer.

This would fix readv/writev issues mentioned in apache#12674. (only for this specific driver though. with this approach, we basically have to fix every single drivers and filesystems.) Lightly tested on the serial console, using micropython REPL on toywasm with esp32s3-devkit:toywasm, which used to be suffered by the readv issue.

xiaoxiang781216 · 2024-12-03T05:50:48Z

fs/vfs/fs_uio.c

 *   Or -EOVERFLOW.
 *
 ****************************************************************************/

-ssize_t uio_total_len(FAR const struct uio *uio)
+ssize_t uio_resid(FAR const struct uio *uio)


Ok, can we follow https://man.freebsd.org/cgi/man.cgi?uio(9) as much as possible, which define uio_offset and uio_resid?

xiaoxiang781216 · 2024-12-03T18:31:10Z

drivers/misc/dev_zero.c

+  total = uio_total_len(uio);
+  if (total >= 0)
+    {
+      uio_advance(uio, total);


can we advance one by one to avoid iterate the vector twice

xiaoxiang781216 · 2024-12-03T18:31:44Z

drivers/misc/dev_zero.c

@@ -94,19 +91,27 @@ static ssize_t devzero_readv(FAR struct file *filep,
      memset(iov[i].iov_base, 0, iov[i].iov_len);
    }

+  uio_advance(uio, total);


can we advance one by one to avoid iterate the vector three times

xiaoxiang781216 · 2024-12-03T18:32:06Z

drivers/loop/loop.c

+  ssize_t ret = uio_total_len(uio);
+  if (ret >= 0)
+    {
+      uio_advance(uio, ret);


can we advance one by one to avoid iterate the vector twice

xiaoxiang781216 · 2024-12-03T18:32:14Z

drivers/misc/dev_null.c

+  ssize_t ret = uio_total_len(uio); /* Say that everything was written */
+  if (ret >= 0)
+    {
+      uio_advance(uio, ret);


can we advance one by one to avoid iterate the vector twice

xiaoxiang781216 · 2024-12-03T18:43:50Z

fs/vfs/fs_uio.c

+      memcpy((FAR uint8_t *)iov->iov_base + offset, buf, blen);
+
+      len -= blen;
+      if (len == 0)


xiaoxiang781216 · 2024-12-03T18:44:25Z

fs/vfs/fs_uio.c

+  DEBUGASSERT(uio_resid(uio) >= 0);
+  DEBUGASSERT(len <= uio_resid(uio));
+  DEBUGASSERT(offset <= uio_resid(uio) - len);
+  DEBUGASSERT(SSIZE_MAX - offset >= uio->uio_offset_in_iov);


xiaoxiang781216 · 2024-12-03T18:44:46Z

fs/vfs/fs_uio.c

+      memcpy(buf, (FAR const uint8_t *)iov->iov_base + offset, blen);
+
+      len -= blen;
+      if (len == 0)


xiaoxiang781216 · 2024-12-03T18:50:04Z

drivers/serial/serial.c

@@ -1408,9 +1469,9 @@ static ssize_t uart_write(FAR struct file *filep, FAR const char *buffer,
   */

  uart_disabletxint(dev);
-  for (; buflen; buflen--)
+  for (; buflen; uio_advance(uio, 1), buflen--)


merge line 1474

xiaoxiang781216 · 2024-12-03T18:51:02Z

drivers/serial/serial.c

      leave_critical_section(flags);

      return ret;
    }

+  buflen = nwritten = uio_resid(uio);
+  if (nwritten < 0)


why add the additional check

github-actions bot added Area: Drivers Drivers issues Area: File System File System issues Size: M The size of the change in this PR is medium labels Nov 22, 2024

yamt force-pushed the readv-serial branch 4 times, most recently from b94fb12 to 2937e17 Compare November 22, 2024 07:06

xiaoxiang781216 reviewed Nov 23, 2024

View reviewed changes

yamt mentioned this pull request Nov 25, 2024

Reimplement readv to deal with short reads #12674

Open

yamt added 5 commits November 27, 2024 16:51

Update a few comments after the recent readv/writev changes

2ef0982

Make readv/writev implementations update struct uio

e8e16f8

This can simplify partial result handling.

rename uio_total_len to uio_resid

1b1fbc4

"total" is a bit confusing after the introduction of uio_advance().

add uio_copyfrom/uio_copyto

9cd9923

although i'm not quite happy with the "offset" functionality, it's necessary to simplify the adaptation of some drivers like drivers/serial/serial.c, which (ab)uses the user-supplied buffer as a line-buffer.

yamt force-pushed the readv-serial branch from 2937e17 to 7e31a26 Compare November 27, 2024 07:57

xiaoxiang781216 reviewed Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/serial/serial.c: adapt to the iovec-based api #14898

drivers/serial/serial.c: adapt to the iovec-based api #14898

yamt commented Nov 22, 2024

nuttxpr commented Nov 22, 2024

xiaoxiang781216 Nov 23, 2024

yamt Nov 25, 2024

yamt Nov 28, 2024

xiaoxiang781216 Dec 3, 2024

yamt Dec 13, 2024

xiaoxiang781216 Nov 23, 2024

yamt Nov 25, 2024

xiaoxiang781216 Dec 3, 2024

yamt Dec 13, 2024

xiaoxiang781216 Dec 3, 2024

xiaoxiang781216 Dec 3, 2024

xiaoxiang781216 Dec 3, 2024

xiaoxiang781216 Dec 3, 2024

xiaoxiang781216 Dec 3, 2024

xiaoxiang781216 Dec 3, 2024

yamt Dec 13, 2024

xiaoxiang781216 Dec 3, 2024

xiaoxiang781216 Dec 3, 2024

yamt Dec 13, 2024

xiaoxiang781216 Dec 3, 2024

xiaoxiang781216 Dec 3, 2024

drivers/serial/serial.c: adapt to the iovec-based api #14898

Are you sure you want to change the base?

drivers/serial/serial.c: adapt to the iovec-based api #14898

Conversation

yamt commented Nov 22, 2024

Summary

Impact

Testing

nuttxpr commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment