fs/vfs: initialize uio only if lower implement readv/writev #15603

anchao · 2025-01-19T11:51:28Z

Summary

fs/vfs: initialize uio only if lower implement readv/writev

to simple signle read/write logic, initialize uio only if lower implement readv/writev

Impact

N/A

Testing

nsh/sim

nuttxpr · 2025-01-19T12:03:32Z

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details. Here's a breakdown:

Summary: The summary is too brief. While it states what is changing, it doesn't adequately explain why. What problem does this solve? What were the drawbacks of the previous approach? What are the benefits of this change? How does initializing uio conditionally simplify the read/write logic? This needs more explanation. Also, any related issues should be linked.
Impact: Simply stating "N/A" is insufficient. Each impact category should be explicitly addressed with "NO" or "YES" and a justification. Even if the answer is "NO," a brief explanation is helpful (e.g., "Impact on user: NO. This is an internal change and does not affect user-facing APIs."). Consider the following:
- Impact on compatibility: Does this change break any existing drivers or applications?
- Impact on performance: Does this change improve or degrade performance?
Testing: The testing section is inadequate.
- Build Host(s): Just stating "nsh/sim" isn't enough. Specify the host OS, CPU architecture, and compiler version. For example: "Linux x86_64, GCC 12.2"
- Target(s): Be more specific. Which simulator? QEMU? Which configuration? Provide details about the simulated hardware. For example: "QEMU-based simulator, Cortex-M4F, default configuration"
- Testing logs: The PR provides placeholders for logs but doesn't include any actual logs. Include logs demonstrating the behavior before and after the change, showcasing the issue being fixed or the new functionality being added. The logs should be relevant to the change and demonstrate that it works as intended.

In short, the PR needs to be more thorough and provide concrete evidence of the change's impact and effectiveness. More detailed explanations and actual testing logs are essential for proper review and acceptance.

fs/vfs/fs_read.c

fs/vfs/fs_uio.c

fs/vfs/fs_write.c

yamt

is this same as a part of #15604?

if yes, i'm against this for the same reason i stated in #15604 (review).

anchao · 2025-01-20T03:20:20Z

Point 1 to answer your question
#15604 (comment)

anchao · 2025-01-20T05:24:01Z

@yamt we could review this PR first, just some uio logic adjustment

yamt · 2025-01-20T06:15:00Z

is it intended to drop the EINVAL check for non-iovec cases? why?
if you just want to save a function call for uio_init, i feel it's simpler to make it a macro and inline the iovcnt == 1 case.

anchao · 2025-01-20T06:26:03Z

is it intended to drop the EINVAL check for non-iovec cases? why?

In the uio case, you need to accumulate the length, but non-iovec not.

if you just want to save a function call for uio_init, i feel it's simpler to make it a macro and inline the iovcnt == 1 case.

any example?

yamt · 2025-01-20T07:04:41Z

is it intended to drop the EINVAL check for non-iovec cases? why?

In the uio case, you need to accumulate the length, but non-iovec not.

a single size_t value alone can exceed SSIZE_MAX.

if you just want to save a function call for uio_init, i feel it's simpler to make it a macro and inline the iovcnt == 1 case.

any example?

yamt@67c9bc6
(not tested at all)

anchao · 2025-01-20T07:19:43Z

yamt@67c9bc6 (not tested at all)

The codes in the red line can be skip all

yamt · 2025-01-20T07:22:39Z

yamt@67c9bc6 (not tested at all)

The codes in the red line can be skip all

"if you just want to save a function call for uio_init"

anchao · 2025-01-20T07:25:32Z

yamt@67c9bc6 (not tested at all)

The codes in the red line can be skip all

"if you just want to save a function call for uio_init"

OK, my opinion has not changed, “uio could be skipped when reading and writing a single vector buffer“, could we reach a consensus on this option?

yamt · 2025-01-20T07:41:05Z

yamt@67c9bc6 (not tested at all)

The codes in the red line can be skip all

"if you just want to save a function call for uio_init"

OK, my opinion has not changed, “uio could be skipped when reading and writing a single vector buffer“, could we reach a consensus on this option?

i agree it's possible.
however, i don't agree it's a good trade-off. IMO, unified api matters more than a small overhead.

btw, do you have any plans on how preadv/pread would look like with your approach?
(asking because i planned to have a file offset in uio)

anchao · 2025-01-20T07:55:35Z

OK, my opinion has not changed, “uio could be skipped when reading and writing a single vector buffer“, could we reach a consensus on this option?

i agree it's possible. however, i don't agree it's a good trade-off. IMO, unified api matters more than a small overhead.

btw, do you have any plans on how preadv/pread would look like with your approach? (asking because i planned to have a file offset in uio)

we can refer to Linux, but the difference from Linux is that in the single vector buffer scenario, we do not use iov_iter(uio):

pread64
|
 ->ksys_pread64
   |
    ->vfs_read
      |
       ->file->f_op->read          /* normal read first */
       ->file->f_op->read_iter
       
preadv
|
 ->do_preadv
   |
    ->vfs_readv
      |
      |->do_iter_readv_writev      /* uio iov_iter first */
      |  |
      |   -> filp->f_op->read_iter
      |  
       ->do_loop_readv_writev      /* normal loop read, uio is unnecessary */
         |
          ->filp->f_op->read

https://github.com/torvalds/linux/blob/master/fs/read_write.c#L1167-L1173

anchao · 2025-01-20T08:47:39Z

i agree it's possible. however, i don't agree it's a good trade-off. IMO, unified api matters more than a small overhead.

I think I may have convinced you a little? Let's go back to this PR, so it is reasonable to provide both uio and non-uio cases in RTOS, right? could you review this PR further?

acassis · 2025-01-20T13:08:03Z

@anchao @yamt I think this solution is good, it avoid overhead for low end MCUs and still working for MCUs that support readv/writev

xiaoxiang781216 · 2025-01-21T08:26:38Z

@yamt do you agree to merge this patch after discussing with @anchao ?

yamt · 2025-01-21T08:48:09Z

@yamt do you agree to merge this patch after discussing with @anchao ?

not really.

i guess the main disagreement between us is on how large the overhead is.
anchao seems to think it important. i feel it's negligible. none of us provided any numbers.
maybe i can do some simple benchmark later.

well, even if the benchmark proves me wrong, i'm against today's version of the PR because it drops the EINVAL check and anchao didn't explain why it was desirable. also, i feel this PR contains too many unrelated changes. (whitespaces, removing assertions for non obvious reasons, etc)

anchao · 2025-01-21T09:40:25Z

@yamt do you agree to merge this patch after discussing with @anchao ?

not really.

i guess the main disagreement between us is on how large the overhead is. anchao seems to think it important. i feel it's negligible. none of us provided any numbers. maybe i can do some simple benchmark later.

I need a balance. You think writev/readv is important and it needs uio. I have no objection. Why must uio be used for read/write? Your modification has inexplicably added a lot of cycle load to everyone's program. Have n't you understand your problem?

well, even if the benchmark proves me wrong, i'm against today's version of the PR because it drops the EINVAL check and anchao didn't explain why it was desirable.

I will bring back EINVAL check, OK?

also, i feel this PR contains too many unrelated changes. (whitespaces, removing assertions for non obvious reasons, etc)

Where is the extra space? Where is non obvious issue? Comment to PR? OK?

anchao · 2025-01-21T10:12:11Z

i guess the main disagreement between us is on how large the overhead is. anchao seems to think it important. i feel it's negligible. none of us provided any numbers. maybe i can do some simple benchmark later.

read test on sim/nsh ubuntu 24.04:

with your patch:

nsh> hello
1:spending 0.32481253s
2:spending 0.27835136s
3:spending 0.27868075s
4:spending 0.28006217s
5:spending 0.28170945s
6:spending 0.28187716s
7:spending 0.27935934s
8:spending 0.28042121s
9:spending 0.28044079s
10:spending 0.28143881s
nsh>

with out your patch( -18% ):

nsh> hello
1:spending 0.28144562s
2:spending 0.23062825s
3:spending 0.23596239s
4:spending 0.23038298s
5:spending 0.22734277s
6:spending 0.22969430s
7:spending 0.22879461s
8:spending 0.22730887s
9:spending 0.22585802s
10:spending 0.22629238s
nsh>

test code:

static void timespec_sub(struct timespec *dest,                            
                         struct timespec *ts1,                             
                         struct timespec *ts2)                             
{                                                                          
  dest->tv_sec = ts1->tv_sec - ts2->tv_sec;                                
  dest->tv_nsec = ts1->tv_nsec - ts2->tv_nsec;                             
                                                                           
  if (dest->tv_nsec < 0)                                                   
    {                                                                      
      dest->tv_nsec += 1000000000;                                         
      dest->tv_sec -= 1;                                                   
    }                                                                      
}                                                                          
                                                                           
                                                                           
int main(int argc, FAR char *argv[])                                       
{                                                                          
  struct timespec result;                                                  
  struct timespec start;                                                   
  struct timespec end;                                                     
  int fd = open("/dev/zero", O_RDONLY);                                    
  int loop = 0;                                                            
  int i = 0;                                                               
  char c;                                                                  
                                                                           
  while (loop++ < 10)                                                      
    {                                                                      
      i = 0;
      clock_gettime(CLOCK_MONOTONIC, &start);                              
      while (i++ < 100000)                                                 
        {                                                                  
          read(fd, &c, 1);                                                 
        }                                                                  
      clock_gettime(CLOCK_MONOTONIC, &end);                                
                                                                           
      timespec_sub(&result, &end, &start);                                 
      printf("%d:spending %d.%lds\n", loop, result.tv_sec, result.tv_nsec);
    }                                                                      
                                                                           
  close(fd);                                                               
  return 0;                                                                
}

yamt · 2025-01-21T10:20:25Z

@yamt do you agree to merge this patch after discussing with @anchao ?

not really.
i guess the main disagreement between us is on how large the overhead is. anchao seems to think it important. i feel it's negligible. none of us provided any numbers. maybe i can do some simple benchmark later.

I need a balance.

i agree on that.

You think writev/readv is important and it needs uio. I have no objection. Why must uio be used for read/write?

because unified logic is easier to maintain.
it isn't always trivial to maintain the consistent behavior if we have multiple paths.
eg. your introduced a bug even with this simple PR. (dropped EINVAL check)

Your modification has inexplicably added a lot of cycle load to everyone's program. Have n't you understand your problem?

well, even if the benchmark proves me wrong, i'm against today's version of the PR because it drops the EINVAL check and anchao didn't explain why it was desirable.

I will bring back EINVAL check, OK?

also, i feel this PR contains too many unrelated changes. (whitespaces, removing assertions for non obvious reasons, etc)

Where is the extra space? Where is non obvious issue? Comment to PR? OK?

i will do so later.

yamt · 2025-01-21T10:22:54Z

i guess the main disagreement between us is on how large the overhead is. anchao seems to think it important. i feel it's negligible. none of us provided any numbers. maybe i can do some simple benchmark later.

read test on sim/nsh ubuntu 24.04:

thank you for the benchmark.
what cpu is it? x86-64?
"without your patch" is this PR and "with your patch" is the base version of this PR?

anchao · 2025-01-21T10:53:01Z

i guess the main disagreement between us is on how large the overhead is. anchao seems to think it important. i feel it's negligible. none of us provided any numbers. maybe i can do some simple benchmark later.

read test on sim/nsh ubuntu 24.04:

thank you for the benchmark. what cpu is it? x86-64? "without your patch" is this PR and "with your patch" is the base version of this PR?

what cpu is it? x86-64?

Yes

$ cat /proc/cpuinfo 
processor	: 0
vendor_id	: GenuineIntel
cpu family	: 6
model		: 151
model name	: 12th Gen Intel(R) Core(TM) i7-12700K
stepping	: 2
microcode	: 0x37
cpu MHz		: 2426.043
cache size	: 25600 KB
physical id	: 0
siblings	: 20
core id		: 0
cpu cores	: 12
apicid		: 0
initial apicid	: 0
fpu		: yes
fpu_exception	: yes
cpuid level	: 32
wp		: yes
flags		: fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc art arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc cpuid aperfmperf tsc_known_freq pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 sdbg fma cx16 xtpr pdcm pcid sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm abm 3dnowprefetch cpuid_fault epb ssbd ibrs ibpb stibp ibrs_enhanced tpr_shadow flexpriority ept vpid ept_ad fsgsbase tsc_adjust bmi1 avx2 smep bmi2 erms invpcid rdseed adx smap clflushopt clwb intel_pt sha_ni xsaveopt xsavec xgetbv1 xsaves split_lock_detect user_shstk avx_vnni dtherm ida arat pln pts hwp hwp_notify hwp_act_window hwp_epp hwp_pkg_req hfi vnmi umip pku ospke waitpkg gfni vaes vpclmulqdq tme rdpid movdiri movdir64b fsrm md_clear serialize pconfig arch_lbr ibt flush_l1d arch_capabilities
vmx flags	: vnmi preemption_timer posted_intr invvpid ept_x_only ept_ad ept_1gb flexpriority apicv tsc_offset vtpr mtf vapic ept vpid unrestricted_guest vapic_reg vid ple shadow_vmcs ept_mode_based_exec tsc_scaling usr_wait_pause
bugs		: spectre_v1 spectre_v2 spec_store_bypass swapgs eibrs_pbrsb rfds bhi
bogomips	: 7219.20
clflush size	: 64
cache_alignment	: 64
address sizes	: 46 bits physical, 48 bits virtual
power management:

...

"without your patch" is this PR and "with your patch" is the base version of this PR?

I just revert writev/readv refactor code:

commit 742865af87ac59bdaf6198c812a475fb02840d31 (HEAD)
Author: chao an <[email protected]>
Date:   Tue Jan 21 18:02:07 2025 +0800

    Revert "fs/vfs: Fix initialization of `g_pseudofile_ops`"
    
    This reverts commit 0702dc536172b73e05a45fe7539ff3df4943ff6d.

commit 665db310055aec9cf9a419229cd94b241a63218d
Author: chao an <[email protected]>
Date:   Tue Jan 21 18:01:19 2025 +0800

    Revert "move readv/writev to the kernel"
    
    This reverts commit 761ee819568a52f1934ccf17bae20fe9816ebdec.

commit 572c813e2bba95bd89fcd7e1afa639d7cf0c25ab
Author: chao an <[email protected]>
Date:   Tue Jan 21 18:00:51 2025 +0800

    Revert "file_readv_compat/file_writev_compat: Fix partial success handling"
    
    This reverts commit 8241a10ebcc259d4b9a9a82337d7124514ffeabd.

commit 38db4703e6f9d94f0b29c258bdc428273ec74571
Author: chao an <[email protected]>
Date:   Tue Jan 21 18:00:42 2025 +0800

    Revert "Update a few comments after the recent readv/writev changes"
    
    This reverts commit 9a2b6be8420ca173d92632a05882e01306e2c41d.

commit eae60045f9e4305ef4e8f617c849625181451c0b
Author: chao an <[email protected]>
Date:   Tue Jan 21 18:00:26 2025 +0800

    Revert "Update a few comments after the recent readv/writev changes"
    
    This reverts commit 2749510413d0015f4fce457de79518f1e6e22712.

commit c2920ff0b494bbf5d792c472ad5d1823a32598c6
Author: chao an <[email protected]>
Date:   Tue Jan 21 18:00:16 2025 +0800

    Revert "uio api tweaks"
    
    This reverts commit 30ad31e9d722556b3bad34457088c3cec46cf988.

commit f927e5bf321d568a28d1cf95e3a43a9150caa031
Author: chao an <[email protected]>
Date:   Tue Jan 21 18:00:11 2025 +0800

    Revert "drivers/serial/serial.c: adapt to the iovec-based api"
    
    This reverts commit 00010089b8332592912d60e57f85f2f954905365.

yamt · 2025-01-21T10:25:13Z

fs/vfs/fs_uio.c

@@ -126,12 +126,14 @@ int uio_init(FAR struct uio *uio, FAR const struct iovec *iov, int iovcnt)
  ssize_t resid;

  memset(uio, 0, sizeof(*uio));
+


unrelated whitespace changes

to me, all changes to this file looks cosmetic and unrelated to the rest of this commit.

just make the code easier to read, let me revert

yamt · 2025-01-21T10:25:44Z

fs/vfs/fs_read.c

  int i;

-  DEBUGASSERT(inode->u.i_ops->read != NULL);


how is this assertion removal related to the rest of this commit? ditto for writev

already have the check before call this internal function:

yamt · 2025-01-21T10:29:26Z

fs/vfs/fs_read.c

 {
  FAR struct inode *inode;
  ssize_t ret = -EBADF;
+  struct uio uio;


i think it's better to move this to where it's used. ie. the if (inode->u.i_ops->readv) block. what do you think?
ditto for writev.

to simple signle read/write logic, initialize uio only if lower implement readv/writev Signed-off-by: chao an <[email protected]>

Signed-off-by: chao an <[email protected]>

anchao · 2025-01-21T12:48:51Z

@yamt please review again

github-actions bot added Area: File System File System issues Size: M The size of the change in this PR is medium labels Jan 19, 2025

anchao force-pushed the 25011901 branch from 0493477 to 883a24b Compare January 19, 2025 11:53

anchao requested a review from yamt January 19, 2025 14:37

xiaoxiang781216 reviewed Jan 20, 2025

View reviewed changes

fs/vfs/fs_read.c Show resolved Hide resolved

fs/vfs/fs_uio.c Outdated Show resolved Hide resolved

fs/vfs/fs_write.c Show resolved Hide resolved

fs/vfs/fs_write.c Show resolved Hide resolved

anchao force-pushed the 25011901 branch 2 times, most recently from c1758db to 7e7b069 Compare January 20, 2025 02:31

yamt requested changes Jan 20, 2025

View reviewed changes

anchao mentioned this pull request Jan 20, 2025

Revert "drivers/serial/serial.c: adapt to the iovec-based api" #15604

Open

acassis approved these changes Jan 20, 2025

View reviewed changes

yamt reviewed Jan 21, 2025

View reviewed changes

anchao added 2 commits January 21, 2025 20:13

fs/vfs: initialize uio only if lower implement readv/writev

ff9addc

to simple signle read/write logic, initialize uio only if lower implement readv/writev Signed-off-by: chao an <[email protected]>

fs/uio: do not overwrite the return value

4fae1b3

Signed-off-by: chao an <[email protected]>

anchao force-pushed the 25011901 branch from 7e7b069 to 4fae1b3 Compare January 21, 2025 12:21

anchao mentioned this pull request Jan 21, 2025

drivers/iovec: revert vector io implement from loop/null/zero driver #15629

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs/vfs: initialize uio only if lower implement readv/writev #15603

fs/vfs: initialize uio only if lower implement readv/writev #15603

anchao commented Jan 19, 2025

nuttxpr commented Jan 19, 2025

yamt left a comment

anchao commented Jan 20, 2025

anchao commented Jan 20, 2025

yamt commented Jan 20, 2025

anchao commented Jan 20, 2025 •

edited

Loading

yamt commented Jan 20, 2025

anchao commented Jan 20, 2025

yamt commented Jan 20, 2025

anchao commented Jan 20, 2025

yamt commented Jan 20, 2025

anchao commented Jan 20, 2025

anchao commented Jan 20, 2025

acassis commented Jan 20, 2025

xiaoxiang781216 commented Jan 21, 2025

yamt commented Jan 21, 2025

anchao commented Jan 21, 2025

anchao commented Jan 21, 2025 •

edited

Loading

yamt commented Jan 21, 2025

yamt commented Jan 21, 2025

anchao commented Jan 21, 2025

yamt Jan 21, 2025

anchao Jan 21, 2025

yamt Jan 21, 2025

anchao Jan 21, 2025

yamt Jan 21, 2025

anchao Jan 21, 2025

anchao commented Jan 21, 2025

		@@ -126,12 +126,14 @@ int uio_init(FAR struct uio uio, FAR const struct iovec iov, int iovcnt)
		ssize_t resid;

		memset(uio, 0, sizeof(*uio));

fs/vfs: initialize uio only if lower implement readv/writev #15603

Are you sure you want to change the base?

fs/vfs: initialize uio only if lower implement readv/writev #15603

Conversation

anchao commented Jan 19, 2025

Summary

Impact

Testing

nuttxpr commented Jan 19, 2025

yamt left a comment

Choose a reason for hiding this comment

anchao commented Jan 20, 2025

anchao commented Jan 20, 2025

yamt commented Jan 20, 2025

anchao commented Jan 20, 2025 • edited Loading

yamt commented Jan 20, 2025

anchao commented Jan 20, 2025

yamt commented Jan 20, 2025

anchao commented Jan 20, 2025

yamt commented Jan 20, 2025

anchao commented Jan 20, 2025

anchao commented Jan 20, 2025

acassis commented Jan 20, 2025

xiaoxiang781216 commented Jan 21, 2025

yamt commented Jan 21, 2025

anchao commented Jan 21, 2025

anchao commented Jan 21, 2025 • edited Loading

yamt commented Jan 21, 2025

yamt commented Jan 21, 2025

anchao commented Jan 21, 2025

yamt Jan 21, 2025

Choose a reason for hiding this comment

anchao Jan 21, 2025

Choose a reason for hiding this comment

yamt Jan 21, 2025

Choose a reason for hiding this comment

anchao Jan 21, 2025

Choose a reason for hiding this comment

yamt Jan 21, 2025

Choose a reason for hiding this comment

anchao Jan 21, 2025

Choose a reason for hiding this comment

anchao commented Jan 21, 2025

anchao commented Jan 20, 2025 •

edited

Loading

anchao commented Jan 21, 2025 •

edited

Loading