(We went off-list just now. Adding lkml again.)
On 5/1/07, Tejun Heo <[email protected]> wrote:
> Satyam Sharma wrote:
> > On 4/28/07, Tejun Heo <[email protected]> wrote:
> >> Implement bin_buffer which contains a mutex and pointer to PAGE_SIZE
> >> buffer to properly synchronize accesses to per-openfile buffer and
> >> prepare for immediate-kobj-disconnect.
> >>
> >> Signed-off-by: Tejun Heo <[email protected]>
> >> ---
> >> [...]
> >> @@ -135,14 +160,22 @@ static int open(struct inode * inode, struct
> >> file * file)
> >> goto Error;
> >>
> >> error = -ENOMEM;
> >> - file->private_data = kmalloc(PAGE_SIZE, GFP_KERNEL);
> >> - if (!file->private_data)
> >> + bb = kzalloc(sizeof(*bb), GFP_KERNEL);
> >> + if (!bb)
> >> goto Error;
> >>
> >> + bb->buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> >
> > You could simply do a __get_free_page() here instead of a
> > kmalloc(PAGE_SIZE).
>
> Yes, I could but 1. the original code was done using kmalloc
Yes, I saw that, but you could change it just as well. In any case,
this will only make fs/sysfs/bin.c similar to what is being done in
fs/sysfs/file.c. We allocate the buffer page backing the attribute's
data in fill_read_buffer() and fill_write_buffer() using
get_zeroed_page() and not kzalloc().
> 2. I'm not sure __get_free_page() is better than kmalloc(PAGE_SIZE, ), is it?
If you know for sure that you require *exactly* PAGE_SIZE bytes, then
yes, it is.
On 5/2/07, Satyam Sharma <[email protected]> wrote:
> (We went off-list just now. Adding lkml again.)
>
> On 5/1/07, Tejun Heo <[email protected]> wrote:
> > Satyam Sharma wrote:
> > > On 4/28/07, Tejun Heo <[email protected]> wrote:
> > >> Implement bin_buffer which contains a mutex and pointer to PAGE_SIZE
> > >> buffer to properly synchronize accesses to per-openfile buffer and
> > >> prepare for immediate-kobj-disconnect.
> > >>
> > >> Signed-off-by: Tejun Heo <[email protected]>
> > >> ---
> > >> [...]
> > >> @@ -135,14 +160,22 @@ static int open(struct inode * inode, struct
> > >> file * file)
> > >> goto Error;
> > >>
> > >> error = -ENOMEM;
> > >> - file->private_data = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > >> - if (!file->private_data)
> > >> + bb = kzalloc(sizeof(*bb), GFP_KERNEL);
> > >> + if (!bb)
> > >> goto Error;
> > >>
> > >> + bb->buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > >
> > > You could simply do a __get_free_page() here instead of a
> > > kmalloc(PAGE_SIZE).
> >
> > Yes, I could but 1. the original code was done using kmalloc
>
> Yes, I saw that, but you could change it just as well. In any case,
> this will only make fs/sysfs/bin.c similar to what is being done in
> fs/sysfs/file.c. We allocate the buffer page backing the attribute's
> data in fill_read_buffer() and fill_write_buffer() using
> get_zeroed_page() and not kzalloc().
Which begs another question -- why do we allocate the buffer page
lazily (only at the time of read(2) and write(2) and not at open(2))
in the case of normal attributes but prefer to do it during open(2)
itself for binary attributes? Note that the page (if allocated) is
freed during release() for both cases.
Satyam Sharma wrote:
>> Yes, I saw that, but you could change it just as well. In any case,
>> this will only make fs/sysfs/bin.c similar to what is being done in
>> fs/sysfs/file.c. We allocate the buffer page backing the attribute's
>> data in fill_read_buffer() and fill_write_buffer() using
>> get_zeroed_page() and not kzalloc().
>
> Which begs another question -- why do we allocate the buffer page
> lazily (only at the time of read(2) and write(2) and not at open(2))
> in the case of normal attributes but prefer to do it during open(2)
> itself for binary attributes? Note that the page (if allocated) is
> freed during release() for both cases.
Well, that also is because that was how bin files did it before the
patch. I don't object to any of your suggestions but also thinks that
they are better off as separate patches. So, feel free to submit patches.
--
tejun