mmap/munmap/ftruncate interface change proposal

[Jukka Laitinen <ju...@iki.fi>]

Hi,

We've implemented and been using "posix shm" support for nuttx ( not 
up-streamed to mainline nuttx so far - I am open to discussion whether 
there is interest for it ). For this, we've naturally utilized the 
interfaces mentioned in the subject.

Currently those operations are implemented as FIOC_ ioctls:s to the 
drivers - such as our shmfs and upstream video, leds and and shmfs.

While this works fine, I can't help thinking that implementing this 
functionality as IOCTLs is wrong, and those should be specified in 
"struct file_operations" instead.

This is my thinking around the subject:

- IOCTLs are ment for communication from userspace application to the 
drivers. It doesn't make sense for an application to perform a direct 
IOCTL (such as FIOC_MMAP, FIOC_MUNMAP or FIOC_TRUNCATE). Application 
should always use posix mmap, munmap and truncate calls instead.

- For mmap, munmap and truncate there is a syscall lookup already in 
place in nuttx - there is no ioctl needed for the userspace-kernel 
communication

- Especially for "unmap", the ioctl doesn't make sense, since file_ioctl 
is supposed to work on a file pointer. But unmap cannot be done on a 
file. Even when unmapping a previously mapped "file", unmap can only 
work on inode, since there may not be an open file, or even a linked 
inode present. It must be possible to do "munmap" for a mapped file, 
even if the actual file is closed and/or deleted after mapping. (and 
yes, I realize that I earlier added the FIOC_MUNMAP myself - since the 
others were implemented as ioctls, and we really need the unmap).

So what do you think, is my reasoning above right, and should the above 
operations moved to file_struct instead of the being IOCTLs?

Br,

Jukka

Re: mmap/munmap/ftruncate interface change proposal

[Jukka Laitinen <jl...@gmail.com>]

Xiang Xiao kirjoitti torstai 22. joulukuuta 2022:
> On Thu, Dec 22, 2022 at 2:28 PM Jukka Laitinen <ju...@iki.fi>
> wrote:
> 
> > Hi,
> >
> > We've implemented and been using "posix shm" support for nuttx ( not
> > up-streamed to mainline nuttx so far - I am open to discussion whether
> > there is interest for it ). For this, we've naturally utilized the
> > interfaces mentioned in the subject.
> >
> >
> Yes, it's a very useful feature.
> 
> 
> > Currently those operations are implemented as FIOC_ ioctls:s to the
> > drivers - such as our shmfs and upstream video, leds and and shmfs.
> >
> > While this works fine, I can't help thinking that implementing this
> > functionality as IOCTLs is wrong, and those should be specified in
> > "struct file_operations" instead.
> >
> >
> You mean FIOC_MMAP? We also find that the rigid design of FIOC_MMAP makes
> it hard to implement some advanced features inside the driver since it
> doesn't map to mmap/mumap very well.
> 
> 
> > This is my thinking around the subject:
> >
> > - IOCTLs are ment for communication from userspace application to the
> > drivers. It doesn't make sense for an application to perform a direct
> > IOCTL (such as FIOC_MMAP, FIOC_MUNMAP or FIOC_TRUNCATE). Application
> > should always use posix mmap, munmap and truncate calls instead.
> >
> > - For mmap, munmap and truncate there is a syscall lookup already in
> > place in nuttx - there is no ioctl needed for the userspace-kernel
> > communication
> >
> > - Especially for "unmap", the ioctl doesn't make sense, since file_ioctl
> > is supposed to work on a file pointer. But unmap cannot be done on a
> > file. Even when unmapping a previously mapped "file", unmap can only
> > work on inode, since there may not be an open file, or even a linked
> > inode present. It must be possible to do "munmap" for a mapped file,
> > even if the actual file is closed and/or deleted after mapping. (and
> > yes, I realize that I earlier added the FIOC_MUNMAP myself - since the
> > others were implemented as ioctls, and we really need the unmap).
> >
> > So what do you think, is my reasoning above right, and should the above
> > operations moved to file_struct instead of the being IOCTLs?
> >
> >
> It's fine to define map/unmap related operation directly in file_operations
> since other OS make the similar decision:
> https://github.com/torvalds/linux/blob/master/include/linux/fs.h#L2102
> https://github.com/torvalds/linux/blob/master/include/linux/mm.h#L540-L612
> 
> But it's important to convert the current fs/driver to utilize the new
> interface to demonstrate that the design is flexible enough to support all
> possible usage.
> 
>

Thanks for your input! I'll provide a PR proposal early next year.

- Jukka
 
> 
> > Br,
> >
> > Jukka
> >
> >
> >
> >
> >
> >
>

Re: mmap/munmap/ftruncate interface change proposal

[Xiang Xiao <xi...@gmail.com>]

On Thu, Dec 22, 2022 at 2:28 PM Jukka Laitinen <ju...@iki.fi>
wrote:

> Hi,
>
> We've implemented and been using "posix shm" support for nuttx ( not
> up-streamed to mainline nuttx so far - I am open to discussion whether
> there is interest for it ). For this, we've naturally utilized the
> interfaces mentioned in the subject.
>
>
Yes, it's a very useful feature.


> Currently those operations are implemented as FIOC_ ioctls:s to the
> drivers - such as our shmfs and upstream video, leds and and shmfs.
>
> While this works fine, I can't help thinking that implementing this
> functionality as IOCTLs is wrong, and those should be specified in
> "struct file_operations" instead.
>
>
You mean FIOC_MMAP? We also find that the rigid design of FIOC_MMAP makes
it hard to implement some advanced features inside the driver since it
doesn't map to mmap/mumap very well.


> This is my thinking around the subject:
>
> - IOCTLs are ment for communication from userspace application to the
> drivers. It doesn't make sense for an application to perform a direct
> IOCTL (such as FIOC_MMAP, FIOC_MUNMAP or FIOC_TRUNCATE). Application
> should always use posix mmap, munmap and truncate calls instead.
>
> - For mmap, munmap and truncate there is a syscall lookup already in
> place in nuttx - there is no ioctl needed for the userspace-kernel
> communication
>
> - Especially for "unmap", the ioctl doesn't make sense, since file_ioctl
> is supposed to work on a file pointer. But unmap cannot be done on a
> file. Even when unmapping a previously mapped "file", unmap can only
> work on inode, since there may not be an open file, or even a linked
> inode present. It must be possible to do "munmap" for a mapped file,
> even if the actual file is closed and/or deleted after mapping. (and
> yes, I realize that I earlier added the FIOC_MUNMAP myself - since the
> others were implemented as ioctls, and we really need the unmap).
>
> So what do you think, is my reasoning above right, and should the above
> operations moved to file_struct instead of the being IOCTLs?
>
>
It's fine to define map/unmap related operation directly in file_operations
since other OS make the similar decision:
https://github.com/torvalds/linux/blob/master/include/linux/fs.h#L2102
https://github.com/torvalds/linux/blob/master/include/linux/mm.h#L540-L612

But it's important to convert the current fs/driver to utilize the new
interface to demonstrate that the design is flexible enough to support all
possible usage.



> Br,
>
> Jukka
>
>
>
>
>
>