On 2020/4/2 上午9:01, David Sterba wrote:
> On Thu, Apr 02, 2020 at 07:31:28AM +0800, Qu Wenruo wrote:
>> On 2020/4/1 下午11:37, David Sterba wrote:
>>> On Thu, Mar 26, 2020 at 04:32:38PM +0800, Qu Wenruo wrote:
>>>> --- a/fs/btrfs/backref.h
>>>> +++ b/fs/btrfs/backref.h
>>>> @@ -78,4 +78,43 @@ struct prelim_ref {
>>>> u64 wanted_disk_byte;
>>>> };
>>>>
>>>> +/*
>>>> + * Helper structure to help iterate backrefs of one extent.
>>>> + *
>>>> + * Now it only supports iteration for tree block in commit root.
>>>> + */
>>>> +struct btrfs_backref_iter {
>>>> + u64 bytenr;
>>>> + struct btrfs_path *path;
>>>> + struct btrfs_fs_info *fs_info;
>>>> + struct btrfs_key cur_key;
>>>> + u32 item_ptr;
>>>> + u32 cur_ptr;
>>>> + u32 end_ptr;
>>>> +};
>>>> +
>>>> +struct btrfs_backref_iter *btrfs_backref_iter_alloc(
>>>> + struct btrfs_fs_info *fs_info, gfp_t gfp_flag);
>>>> +
>>>> +static inline void btrfs_backref_iter_free(struct btrfs_backref_iter *iter)
>>>> +{
>>>> + if (!iter)
>>>> + return;
>>>> + btrfs_free_path(iter->path);
>>>> + kfree(iter);
>>>> +}
>>>
>>> Why do you make so many functions static inline? It makes sense for some
>>> of them but in the following patches there are functions that are either
>>> too big (so when they're inlined it bloats the asm) or called
>>> infrequently so the inlining does not bring much. Code in header files
>>> should be kept to minimum.
>>
>> As most of them meet the requirement for either too small, or too
>> infrequently called.
>
> So the rules or recommendations I use to decide if a function should be
> static inline:
>
> * it's like macro with type checking
> * results into a few instructions (where few is like 3-6)
> * the function is good for code readability, like for a helper that does
> some checks and returns a result, or derefernces a few pointers, and
> the function name is self explaining
After re-checking the backref.h, I still find most (if not all) of these
inlined functions meet these conditions.
They are short, mostly just doing basic check then free up some pointers.
> * there should be some performance reason where the function call would
> be too costly, eg. if the function is on a hot path or in a loop
This is my main concern.
Compiler can easily "uninline" functions if they are only static inline
inside the same C file.
But if we define some function in headers, then it will always be a
function call, and can't be "uninlined"
That's my major concern, any exported functions is a barrier where
compiler can't do their best to optimize.
Thus to me, I agree condition 1~3, but no the 4th if we're handling
exported functions, as it's a single directional optimization.
If there is some proof (I don't believe though) that, compiler can
uninline/inline such exported functions, then I'm completely happy to
make them regular functions.
>
> And all of that can be irrelevant if the compiler does some fancy
> optimization, like function cloning where it keeps one copy intact
> that's for the public interface and then inline other copies, possibly
> applying more optimizations eg. based on parameters or some analysis
> that splits function to hot and cold parts.
>
> Unless we find some suboptimal result of compilation that could be fixed
> by static inlines, I tend to not use them besides the trivial cases that
> help code readability.
But these functions are small, self explaining, thus rarely get read
that much.
>
>>> There are also functions not used anywhere else than in backref.c so
>>> they don't need to be exported for now. For example
>>> btrfs_backref_iter_is_inline_ref.
>>
>> But it's used in later patches, thus I exported them to avoid re-export
>> them later.
>
> I grepped the whole branch with the backref cache and assumed that if
> you introduce a function in the cleanup part, it would be used in the
> other one. But btrfs_backref_iter_is_inline_ref wasn't.
Oh, you're right. That function is only temporary used.
It's introduced in patch 02, then utilized in patch 03, where the major
backref cache handling is still in relocation.c.
Then in patch 29, the backref cache code got moved to backref.c, then no
one utilize that function outside of backref.c, thus it no longer needs
to be exported.
Would you mind to unexport it in patch 29 too?
Thanks,
Qu
>
>>>> +int btrfs_backref_iter_start(struct btrfs_backref_iter *iter, u64 bytenr);
>>>> +
>>>> +static inline void
>>>> +btrfs_backref_iter_release(struct btrfs_backref_iter *iter)
>>>
>>> Please keep the function type and name on the same line, arguments can
>>> go to the next line.
>>
>> Forgot this one...
>>
>> Do I need to resend?
>
> I fix such things when applying the patches so for that only reason it's
> not necessary to resend.
>
