Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+348 −75
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ef0982
Update a few comments after the recent readv/writev changes
yamt Nov 21, 2024
e8e16f8
Make readv/writev implementations update struct uio
yamt Nov 21, 2024
1b1fbc4
rename uio_total_len to uio_resid
yamt Nov 22, 2024
9cd9923
add uio_copyfrom/uio_copyto
yamt Nov 21, 2024
7e31a26
drivers/serial/serial.c: adapt to the iovec-based api
yamt Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions drivers/loop/loop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,8 @@
* Private Function Prototypes
****************************************************************************/

static ssize_t loop_readv(FAR struct file *filep,
FAR const struct uio *uio);
static ssize_t loop_writev(FAR struct file *filep,
FAR const struct uio *uio);
static ssize_t loop_readv(FAR struct file *filep, FAR struct uio *uio);
static ssize_t loop_writev(FAR struct file *filep, FAR struct uio *uio);
static int loop_ioctl(FAR struct file *filep, int cmd,
unsigned long arg);

Expand Down Expand Up @@ -74,8 +72,7 @@ static const struct file_operations g_loop_fops =
* Name: loop_readv
****************************************************************************/

static ssize_t loop_readv(FAR struct file *filep,
FAR const struct uio *uio)
static ssize_t loop_readv(FAR struct file *filep, FAR struct uio *uio)
{
return 0; /* Return EOF */
}
Expand All @@ -84,10 +81,17 @@ static ssize_t loop_readv(FAR struct file *filep,
* Name: loop_writev
****************************************************************************/

static ssize_t loop_writev(FAR struct file *filep,
FAR const struct uio *uio)
static ssize_t loop_writev(FAR struct file *filep, FAR struct uio *uio)
{
return uio_total_len(uio); /* Say that everything was written */
/* Say that everything was written */

ssize_t ret = uio_total_len(uio);
if (ret >= 0)
{
uio_advance(uio, ret);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return ret;
}

/****************************************************************************
Expand Down
20 changes: 11 additions & 9 deletions drivers/misc/dev_null.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,8 @@
* Private Function Prototypes
****************************************************************************/

static ssize_t devnull_readv(FAR struct file *filep,
FAR const struct uio *uio);
static ssize_t devnull_writev(FAR struct file *filep,
FAR const struct uio *uio);
static ssize_t devnull_readv(FAR struct file *filep, FAR struct uio *uio);
static ssize_t devnull_writev(FAR struct file *filep, FAR struct uio *uio);
static int devnull_poll(FAR struct file *filep, FAR struct pollfd *fds,
bool setup);

Expand Down Expand Up @@ -74,8 +72,7 @@ static const struct file_operations g_devnull_fops =
* Name: devnull_readv
****************************************************************************/

static ssize_t devnull_readv(FAR struct file *filep,
FAR const struct uio *uio)
static ssize_t devnull_readv(FAR struct file *filep, FAR struct uio *uio)
{
UNUSED(filep);
UNUSED(uio);
Expand All @@ -87,12 +84,17 @@ static ssize_t devnull_readv(FAR struct file *filep,
* Name: devnull_writev
****************************************************************************/

static ssize_t devnull_writev(FAR struct file *filep,
FAR const struct uio *uio)
static ssize_t devnull_writev(FAR struct file *filep, FAR struct uio *uio)
{
UNUSED(filep);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return ret;
}

/****************************************************************************
Expand Down
23 changes: 14 additions & 9 deletions drivers/misc/dev_zero.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,8 @@
* Private Function Prototypes
****************************************************************************/

static ssize_t devzero_readv(FAR struct file *filep,
FAR const struct uio *uio);
static ssize_t devzero_writev(FAR struct file *filep,
FAR const struct uio *uio);
static ssize_t devzero_readv(FAR struct file *filep, FAR struct uio *uio);
static ssize_t devzero_writev(FAR struct file *filep, FAR struct uio *uio);
static int devzero_poll(FAR struct file *filep, FAR struct pollfd *fds,
bool setup);

Expand Down Expand Up @@ -74,8 +72,7 @@ static const struct file_operations g_devzero_fops =
* Name: devzero_readv
****************************************************************************/

static ssize_t devzero_readv(FAR struct file *filep,
FAR const struct uio *uio)
static ssize_t devzero_readv(FAR struct file *filep, FAR struct uio *uio)
{
ssize_t total = uio_total_len(uio);
FAR const struct iovec *iov = uio->uio_iov;
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return total;
}

/****************************************************************************
* Name: devzero_writev
****************************************************************************/

static ssize_t devzero_writev(FAR struct file *filep,
FAR const struct uio *uio)
static ssize_t devzero_writev(FAR struct file *filep, FAR struct uio *uio)
{
ssize_t total;
UNUSED(filep);

return uio_total_len(uio);
total = uio_total_len(uio);
if (total >= 0)
{
uio_advance(uio, total);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return total;
}

/****************************************************************************
Expand Down
12 changes: 9 additions & 3 deletions fs/vfs/fs_read.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@
*
****************************************************************************/

static ssize_t file_readv_compat(FAR struct file *filep,
FAR const struct uio *uio)
static ssize_t file_readv_compat(FAR struct file *filep, FAR struct uio *uio)
{
FAR const struct iovec *iov = uio->uio_iov;
int iovcnt = uio->uio_iovcnt;
Expand Down Expand Up @@ -102,6 +101,11 @@ static ssize_t file_readv_compat(FAR struct file *filep,
remaining -= nread;
}

if (ntotal >= 0)
{
uio_advance(uio, ntotal);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it eases logic like:

while (uio_resid(uio) > 0) {

   n = process_some(uio, ...);

   uio_advance(uio, n);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return ntotal;
}

Expand Down Expand Up @@ -130,7 +134,7 @@ static ssize_t file_readv_compat(FAR struct file *filep,
*
****************************************************************************/

ssize_t file_readv(FAR struct file *filep, FAR const struct uio *uio)
ssize_t file_readv(FAR struct file *filep, FAR struct uio *uio)
{
FAR struct inode *inode;
ssize_t ret = -EBADF;
Expand Down Expand Up @@ -207,6 +211,7 @@ ssize_t file_read(FAR struct file *filep, FAR void *buf, size_t nbytes)

iov.iov_base = buf;
iov.iov_len = nbytes;
uio_init(&uio);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add iov and count argument to uio_init?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess we can. let me think a bit.

uio.uio_iov = &iov;
uio.uio_iovcnt = 1;
return file_readv(filep, &uio);
Expand Down Expand Up @@ -251,6 +256,7 @@ ssize_t nx_readv(int fd, FAR const struct iovec *iov, int iovcnt)

/* Then let file_readv do all of the work. */

uio_init(&uio);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

uio.uio_iov = iov;
uio.uio_iovcnt = iovcnt;
ret = file_readv(filep, &uio);
Expand Down
55 changes: 53 additions & 2 deletions fs/vfs/fs_uio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,69 @@ ssize_t uio_total_len(FAR const struct uio *uio)
{
const struct iovec *iov = uio->uio_iov;
int iovcnt = uio->uio_iovcnt;
size_t offset_in_iov = uio->uio_offset_in_iov;
size_t len = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change ssIze_t

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can be ssize_t, sure.

otoh, writing code with signed arithmetic uses my brain cycles more.
and from my experience, sometimes compilers produce less efficient machine code.

let me think a bit.

int i;

for (i = 0; i < iovcnt; i++)
{
if (SSIZE_MAX - len < iov[i].iov_len)
DEBUGASSERT(offset_in_iov <= iov[i].iov_len);
if (SSIZE_MAX - len < iov[i].iov_len - offset_in_iov)
{
return -EOVERFLOW;
}

len += iov[i].iov_len;
len += iov[i].iov_len - offset_in_iov;
offset_in_iov = 0;
xiaoxiang781216 marked this conversation as resolved.
Show resolved Hide resolved
}

return len;
}

/****************************************************************************
* Name: uio_advance
*
* Description:
* Advance the pointer/offset in uio by the specified amount.
*
****************************************************************************/

void uio_advance(FAR struct uio *uio, size_t sz)
{
FAR const struct iovec *iov = uio->uio_iov;
int iovcnt = uio->uio_iovcnt;
size_t offset_in_iov = uio->uio_offset_in_iov;

DEBUGASSERT(sz <= uio_total_len(uio));
while (iovcnt > 0)
{
DEBUGASSERT(offset_in_iov <= iov->iov_len);
if (sz < iov->iov_len - offset_in_iov)
{
offset_in_iov += sz;
break;
}

sz -= iov->iov_len;
iov++;
xiaoxiang781216 marked this conversation as resolved.
Show resolved Hide resolved
iovcnt--;
offset_in_iov = 0;
}

uio->uio_iov = iov;
uio->uio_iovcnt = iovcnt;
uio->uio_offset_in_iov = offset_in_iov;
}

/****************************************************************************
* Name: uio_init
*
* Description:
* Initialize the uio structure with reasonable default values.
*
****************************************************************************/

void uio_init(FAR struct uio *uio)
{
memset(uio, 0, sizeof(*uio));
}
11 changes: 9 additions & 2 deletions fs/vfs/fs_write.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
****************************************************************************/

static ssize_t file_writev_compat(FAR struct file *filep,
FAR const struct uio *uio)
FAR struct uio *uio)
{
FAR const struct iovec *iov = uio->uio_iov;
int iovcnt = uio->uio_iovcnt;
Expand Down Expand Up @@ -102,6 +102,11 @@ static ssize_t file_writev_compat(FAR struct file *filep,
remaining -= nwritten;
}

if (ntotal >= 0)
{
uio_advance(uio, ntotal);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move into for loop

}

return ntotal;
}

Expand Down Expand Up @@ -133,7 +138,7 @@ static ssize_t file_writev_compat(FAR struct file *filep,
*
****************************************************************************/

ssize_t file_writev(FAR struct file *filep, FAR const struct uio *uio)
ssize_t file_writev(FAR struct file *filep, FAR struct uio *uio)
{
FAR struct inode *inode;
ssize_t ret = -EBADF;
Expand Down Expand Up @@ -205,6 +210,7 @@ ssize_t file_write(FAR struct file *filep, FAR const void *buf,

iov.iov_base = (FAR void *)buf;
iov.iov_len = nbytes;
uio_init(&uio);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

uio.uio_iov = &iov;
uio.uio_iovcnt = 1;
return file_writev(filep, &uio);
Expand Down Expand Up @@ -252,6 +258,7 @@ ssize_t nx_writev(int fd, FAR const struct iovec *iov, int iovcnt)
* index. Note that file_writev() will return the errno on failure.
*/

uio_init(&uio);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

uio.uio_iov = iov;
uio.uio_iovcnt = iovcnt;
ret = file_writev(filep, &uio);
Expand Down
12 changes: 6 additions & 6 deletions include/nuttx/fs/fs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,8 @@ struct file_operations

CODE int (*poll)(FAR struct file *filep, FAR struct pollfd *fds,
bool setup);
CODE ssize_t (*readv)(FAR struct file *filep, FAR const struct uio *uio);
CODE ssize_t (*writev)(FAR struct file *filep, FAR const struct uio *uio);
CODE ssize_t (*readv)(FAR struct file *filep, FAR struct uio *uio);
CODE ssize_t (*writev)(FAR struct file *filep, FAR struct uio *uio);

/* The two structures need not be common after this point */

Expand Down Expand Up @@ -310,8 +310,8 @@ struct mountpt_operations
CODE int (*truncate)(FAR struct file *filep, off_t length);
CODE int (*poll)(FAR struct file *filep, FAR struct pollfd *fds,
bool setup);
CODE ssize_t (*readv)(FAR struct file *filep, FAR const struct uio *uio);
CODE ssize_t (*writev)(FAR struct file *filep, FAR const struct uio *uio);
CODE ssize_t (*readv)(FAR struct file *filep, FAR struct uio *uio);
CODE ssize_t (*writev)(FAR struct file *filep, FAR struct uio *uio);

/* The two structures need not be common after this point. The following
* are extended methods needed to deal with the unique needs of mounted
Expand Down Expand Up @@ -1428,7 +1428,7 @@ int close_mtddriver(FAR struct inode *pinode);
****************************************************************************/

ssize_t file_read(FAR struct file *filep, FAR void *buf, size_t nbytes);
ssize_t file_readv(FAR struct file *filep, FAR const struct uio *uio);
ssize_t file_readv(FAR struct file *filep, FAR struct uio *uio);

/****************************************************************************
* Name: nx_read
Expand Down Expand Up @@ -1482,7 +1482,7 @@ ssize_t nx_readv(int fd, FAR const struct iovec *iov, int iovcnt);

ssize_t file_write(FAR struct file *filep, FAR const void *buf,
size_t nbytes);
ssize_t file_writev(FAR struct file *filep, FAR const struct uio *uio);
ssize_t file_writev(FAR struct file *filep, FAR struct uio *uio);

/****************************************************************************
* Name: nx_write
Expand Down
21 changes: 21 additions & 0 deletions include/nuttx/fs/uio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ struct uio
{
FAR const struct iovec *uio_iov;
int uio_iovcnt;
size_t uio_offset_in_iov; /* offset in uio_iov[0].iov_base */
xiaoxiang781216 marked this conversation as resolved.
Show resolved Hide resolved
};

/****************************************************************************
Expand All @@ -62,4 +63,24 @@ struct uio

ssize_t uio_total_len(FAR const struct uio *uio);

/****************************************************************************
* Name: uio_advance
*
* Description:
* Advance the pointer/offset in uio by the specified amount.
*
****************************************************************************/

void uio_advance(FAR struct uio *uio, size_t sz);

/****************************************************************************
* Name: uio_init
*
* Description:
* Initialize the uio structure with reasonable default values.
*
****************************************************************************/

void uio_init(FAR struct uio *uio);

#endif /* __INCLUDE_NUTTX_FS_UIO_H */