nuttx icon indicating copy to clipboard operation
nuttx copied to clipboard
verified publisher icon apache

WIP: move readv/writev to the kernel

Open yamt opened this issue 1 year ago • 5 comments

Summary

currently, nuttx implements readv/writev on the top of read/write. while it might work for the simplest cases, it's broken by design. for example, it's impossible to make it work correctly for files which need to preserve data boundaries without allocating a single contiguous buffer. (udp socket, some character devices, etc)

this change is a start of the migration to a better design. that is, implement read/write on the top of readv/writev.

to avoid a single huge change, following things will NOT be done in this PR:

  • fix actual bugs caused by the original readv-based-on-read design (cf. https://github.com/apache/nuttx/pull/12674)
  • adapt filesystems/drivers (except a few trivial examples)
  • retire read/write syscalls. implement them in libc instead.
  • pread/pwrite/preadv/pwritev (except the introduction of struct uio)

Impact

Testing

yamt avatar Sep 17 '24 08:09 yamt

@xiaoxiang781216 can you please review this before i apply the mechanical change to hundreds of files? thank you.

yamt avatar Sep 17 '24 10:09 yamt

@xiaoxiang781216 do you prefer the latest push?

yamt avatar Sep 18 '24 00:09 yamt

i wonder what caused this error on the CI. i couldn't reproduce it with local docker container using the same ghcr.io/apache/nuttx/apache-nuttx-ci-linux image.

FAILED: nuttx 
: && /tools/ccache/bin/gcc  -Wl,--gc-sections -Wl,-Ttext-segment=0x40000000 -T nuttx.ld CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostirq.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostmemory.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostmisc.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hosttime.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostuart.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostsmp.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostfs.c.o CMakeFiles/nuttx.dir/empty.c.o -o nuttx  nuttx.rel  -lpthread  -lz  -lrt  -lm && :
/usr/bin/ld: nuttx.rel: in function `loop_writev':
/github/workspace/sources/nuttx/drivers/loop/loop.c:89: undefined reference to `iovec_total_len'
/usr/bin/ld: nuttx.rel: in function `devnull_writev':
/github/workspace/sources/nuttx/drivers/misc/dev_null.c:94: undefined reference to `iovec_total_len'
/usr/bin/ld: nuttx.rel: in function `devzero_readv':
/github/workspace/sources/nuttx/drivers/misc/dev_zero.c:79: undefined reference to `iovec_total_len'
/usr/bin/ld: nuttx.rel: in function `devzero_writev':
/github/workspace/sources/nuttx/drivers/misc/dev_zero.c:108: undefined reference to `iovec_total_len'
/usr/bin/ld: nuttx.rel: in function `file_readv':
/github/workspace/sources/nuttx/fs/vfs/fs_read.c:97: undefined reference to `iovec_compat_readv'
/usr/bin/ld: nuttx.rel: in function `file_writev':
/github/workspace/sources/nuttx/fs/vfs/fs_write.c:93: undefined reference to `iovec_compat_writev'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
cp: cannot stat 'nuttx': No such file or directory

yamt avatar Oct 18 '24 07:10 yamt

i wonder what caused this error on the CI. i couldn't reproduce it with local docker container using the same ghcr.io/apache/nuttx/apache-nuttx-ci-linux image.

FAILED: nuttx 
: && /tools/ccache/bin/gcc  -Wl,--gc-sections -Wl,-Ttext-segment=0x40000000 -T nuttx.ld CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostirq.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostmemory.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostmisc.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hosttime.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostuart.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostsmp.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostfs.c.o CMakeFiles/nuttx.dir/empty.c.o -o nuttx  nuttx.rel  -lpthread  -lz  -lrt  -lm && :
/usr/bin/ld: nuttx.rel: in function `loop_writev':
/github/workspace/sources/nuttx/drivers/loop/loop.c:89: undefined reference to `iovec_total_len'
/usr/bin/ld: nuttx.rel: in function `devnull_writev':
/github/workspace/sources/nuttx/drivers/misc/dev_null.c:94: undefined reference to `iovec_total_len'
/usr/bin/ld: nuttx.rel: in function `devzero_readv':
/github/workspace/sources/nuttx/drivers/misc/dev_zero.c:79: undefined reference to `iovec_total_len'
/usr/bin/ld: nuttx.rel: in function `devzero_writev':
/github/workspace/sources/nuttx/drivers/misc/dev_zero.c:108: undefined reference to `iovec_total_len'
/usr/bin/ld: nuttx.rel: in function `file_readv':
/github/workspace/sources/nuttx/fs/vfs/fs_read.c:97: undefined reference to `iovec_compat_readv'
/usr/bin/ld: nuttx.rel: in function `file_writev':
/github/workspace/sources/nuttx/fs/vfs/fs_write.c:93: undefined reference to `iovec_compat_writev'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
cp: cannot stat 'nuttx': No such file or directory

this was because of missing changes to cmakefile. fixed.

yamt avatar Oct 21 '24 12:10 yamt

i will probably rebase to master and squash commits later. other than that, this is ready to review.

yamt avatar Oct 21 '24 12:10 yamt

i will probably rebase to master and squash commits later.

done

yamt avatar Oct 23 '24 06:10 yamt

@xiaoxiang781216 thank you for your earlier feedback. now this PR is ready for wider review.

yamt avatar Oct 23 '24 06:10 yamt

@xiaoxiang781216 thank you for your earlier feedback. now this PR is ready for wider review.

thanks to make a general improve to vfs layer, which fix a major drawback, I will review the change in the weekend.

xiaoxiang781216 avatar Oct 23 '24 09:10 xiaoxiang781216

the ci failure looks unrelated.

https://github.com/apache/nuttx/actions/runs/11546452210/job/32134877419?pr=13498

curl: (28) Failed to connect to github.com port 443 after 135159 ms: Connection timed out

yamt avatar Oct 28 '24 03:10 yamt

@yamt please squash the comment change to the first patch.

xiaoxiang781216 avatar Oct 28 '24 14:10 xiaoxiang781216

@yamt please squash the comment change to the first patch.

done

yamt avatar Oct 29 '24 07:10 yamt

@yamt just one minor comment, let's squash the patch, I will merge it after ci pass.

xiaoxiang781216 avatar Oct 30 '24 04:10 xiaoxiang781216

@yamt just one minor comment, let's squash the patch, I will merge it after ci pass.

squashed

yamt avatar Oct 30 '24 06:10 yamt

Hi folks!

Just ran the internal Espressif CI on this commit and the one before it. Seems this one broke our TWAI (CAN) and RMT tests across multiple devices.

TWAI error output: can -n 5 -b 5 timedout! RMT error output: rmtchar -i 129 timedout!

fdcavalcanti avatar Oct 30 '24 18:10 fdcavalcanti

Hi folks!

Just ran the internal Espressif CI on this commit and the one before it. Seems this one broke our TWAI (CAN) and RMT tests across multiple devices.

TWAI error output: can -n 5 -b 5 timedout! RMT error output: rmtchar -i 129 timedout!

what do those tests do?

yamt avatar Oct 31 '24 03:10 yamt

Those are both loopback tests.

Here's the output on ESP32C3 when using TWAI defconfig with loopback enabled.

Configuration used:

  • ./tools/configure.sh -l esp32c3-generic:twai
  • Set: CONFIG_DEBUG_ASSERTIONS=y CONFIG_DEBUG_ASSERTIONS_EXPRESSION=y CONFIG_DEBUG_FEATURES=y CONFIG_CAN_LOOPBACK=y CONFIG_ESPRESSIF_TWAI_TEST_MODE=y CONFIG_ESPRESSIF_TWAI0_TXPIN=8 CONFIG_ESPRESSIF_TWAI0_RXPIN=8
  • make and flash

Expected output

Hash: e3d7d236189517129b66557342ed1273c84f8ab8

nsh> can -n 5 -b 5
nmsgs: 5
min ID: 1 max ID: 5
Bit timing:
   Baud: 1818181
  TSEG1: 16
  TSEG2: 5
    SJW: 4
  ID:    1 DLC: 1
  ID:    1 DLC: 1
  ID:    1 DLC: 1 -- OK
  ID:    2 DLC: 2
  ID:    2 DLC: 2
  ID:    2 DLC: 2 -- OK
  ID:    3 DLC: 3
  ID:    3 DLC: 3
  ID:    3 DLC: 3 -- OK
  ID:    4 DLC: 4
  ID:    4 DLC: 4
  ID:    4 DLC: 4 -- OK
  ID:    5 DLC: 5
  ID:    5 DLC: 5
  ID:    5 DLC: 5 -- OK
Terminating!

Current output

Hash: 761ee819568a52f1934ccf17bae20fe9816ebdec (this PR)

nsh> can -n 5 -b 5
nmsgs: 5
min ID: 1 max ID: 5
Bit timing:
   Baud: 1818181
  TSEG1: 16
  TSEG2: 5
    SJW: 4
  ID:    1 DLC: 1

Stops responding at this point

fdcavalcanti avatar Oct 31 '24 11:10 fdcavalcanti

Those are both loopback tests.

Here's the output on ESP32C3 when using TWAI defconfig with loopback enabled.

Configuration used:

* `./tools/configure.sh -l esp32c3-generic:twai`

* Set:
  CONFIG_DEBUG_ASSERTIONS=y
  CONFIG_DEBUG_ASSERTIONS_EXPRESSION=y
  CONFIG_DEBUG_FEATURES=y
  CONFIG_CAN_LOOPBACK=y
  CONFIG_ESPRESSIF_TWAI_TEST_MODE=y
  CONFIG_ESPRESSIF_TWAI0_TXPIN=8
  CONFIG_ESPRESSIF_TWAI0_RXPIN=8

* make and flash

Expected output

Hash: e3d7d23

nsh> can -n 5 -b 5
nmsgs: 5
min ID: 1 max ID: 5
Bit timing:
   Baud: 1818181
  TSEG1: 16
  TSEG2: 5
    SJW: 4
  ID:    1 DLC: 1
  ID:    1 DLC: 1
  ID:    1 DLC: 1 -- OK
  ID:    2 DLC: 2
  ID:    2 DLC: 2
  ID:    2 DLC: 2 -- OK
  ID:    3 DLC: 3
  ID:    3 DLC: 3
  ID:    3 DLC: 3 -- OK
  ID:    4 DLC: 4
  ID:    4 DLC: 4
  ID:    4 DLC: 4 -- OK
  ID:    5 DLC: 5
  ID:    5 DLC: 5
  ID:    5 DLC: 5 -- OK
Terminating!

Current output

Hash: 761ee81 (this PR)

nsh> can -n 5 -b 5
nmsgs: 5
min ID: 1 max ID: 5
Bit timing:
   Baud: 1818181
  TSEG1: 16
  TSEG2: 5
    SJW: 4
  ID:    1 DLC: 1

Stops responding at this point

thank you. i now have an idea about the cause. i will submit a fix later.

yamt avatar Nov 01 '24 04:11 yamt

i will submit a fix later.

https://github.com/apache/nuttx/pull/14597

yamt avatar Nov 01 '24 11:11 yamt

i will submit a fix later.

#14597

I tested the #14597 branch and seems to fix the problem on Espressif's CI. Thanks @yamt

fdcavalcanti avatar Nov 04 '24 12:11 fdcavalcanti

i will submit a fix later.

#14597

I tested the #14597 branch and seems to fix the problem on Espressif's CI. Thanks @yamt

thank you for testing.

yamt avatar Nov 05 '24 08:11 yamt