Skip to content

Conversation

@crafcat7
Copy link
Contributor

Summary

  1. Add lib_get_pathbuffer / lib_put_pathbuffer instead of temporary variable.
  2. Reduce stack size. Getting the tmp buffer through the lib_get_pathbuffer function reduces the declaration of temporary variables
  3. Fix an unsynchronized lock acquisition by closelk in multi-threaded scenarios. Ensure atomic behavior when acquiring lock status.
  4. Optimized additional performance overhead due to closelk in the close phase (even if the fd doesn't use fslock)
    Optimizing performance:
    Before: Time taken to close the file: 33984 nsec
    After: Time taken to close the file: 23744 nsec
    Improvement of about 10 msec

Impact

Optimizing fslock

Testing

Local test pass

Copy link
Contributor

@PetervdPerk-NXP PetervdPerk-NXP left a comment

Choose a reason for hiding this comment

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

You're trading stack usage for heap fragmentation.
I disagree with this approach because once your heap is to fragmented malloc would simply fail and your system most likely crashes.
if you really want it I propose you make an optional kconfig symbol called i.e. dynamic path allocation

@crafcat7
Copy link
Contributor Author

You're trading stack usage for heap fragmentation. I disagree with this approach because once your heap is to fragmented malloc would simply fail and your system most likely crashes. if you really want it I propose you make an optional kconfig symbol called i.e. dynamic path allocation

lib_get_pathbuffer will initially request an address from a global stack, just like we use temporary variables, which will not cause memory fragmentation. Unless the current usage peak exceeds the available buffer, it will request from the heap.

I think the fragmentation problem can be tuned with CONFIG_LIBC_MAX_PATHBUFFER, which can reduce/avoid malloc from heap

@guohao15
Copy link
Contributor

You're trading stack usage for heap fragmentation. I disagree with this approach because once your heap is to fragmented malloc would simply fail and your system most likely crashes. if you really want it I propose you make an optional kconfig symbol called i.e. dynamic path allocation

We can use LIBC_MAX_PATHBUFFER Kconfig to optimise the useasge of this buffer, reduce the chance that system alloc memory form heap .

@xiaoxiang781216
Copy link
Contributor

You're trading stack usage for heap fragmentation.

the allocation doesn't happen if the concurrent usage doesn't exceed CONFIG_LIBC_MAX_PATHBUFFER.

I disagree with this approach because once your heap is to fragmented malloc would simply fail and your system most likely crashes. if you really want it I propose you make an optional kconfig symbol called i.e. dynamic path allocation

if you want, we can add an option to skip the allocation and return NULL directly if the usage exceeds CONFIG_LIBC_MAX_PATHBUFFER.

@PetervdPerk-NXP
Copy link
Contributor

PetervdPerk-NXP commented Aug 19, 2024

if you want, we can add an option to skip the allocation and return NULL directly if the usage exceeds CONFIG_LIBC_MAX_PATHBUFFER.

Yeah that sounds more logical to me. I don't like the idea of hidden malloc's.

@xiaoxiang781216
Copy link
Contributor

if you want, we can add an option to skip the allocation and return NULL directly if the usage exceeds CONFIG_LIBC_MAX_PATHBUFFER.

Yeah that sounds more logical to me. I don't like the idea of hidden malloc's.

OK, @crafcat7 will create a new patch to forbid the allocation.

@crafcat7
Copy link
Contributor Author

if you want, we can add an option to skip the allocation and return NULL directly if the usage exceeds CONFIG_LIBC_MAX_PATHBUFFER.

Yeah that sounds more logical to me. I don't like the idea of hidden malloc's.

OK, @crafcat7 will create a new patch to forbid the allocation.

Done

@xiaoxiang781216
Copy link
Contributor

@crafcat7 please fix the conflict.

crafcat7 and others added 5 commits August 21, 2024 11:12
Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
…et may be deleted by other factors during the search process, resulting in assertion.

hsearch_r(item = (key = 0x61492174,data = ?),action = ?, retval = 0x6266AC44, htab = 0x6159B758) H 1tem = Ckey = 0x61492174, data= :
 · action = ?
  retval = 0x6266AC44
  htab = 0x6159B758
  haad - 0x61616300
  ie= 0×38

file_lock_find_bucket(inline)
  filepath = 0x61492174
  hretvalue = 0x6055991D
  item = (key = 0x61492174, data = 0x0)

Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
Summary:
  Indicate whether the file is currently locked by adding a new field locked to filep.
    0 - Unlocked
    1 - Locked
  The status of the filep at close is used to determine whether to continue with the following procedure.

  Optimizing performance:
Before
  Time taken to close the file: 33984 nsec
After
  Time taken to close the file: 23744 nsec
Improvement of about 10 msec

Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
Signed-off-by: guohao15 <guohao15@xiaomi.com>
…LIBC_PATHBUFFER_MALLOC

Summary:
  If we disable LIB_PATHBUFFER_MALLOC, that when the path buffer is insufficient, NULL is returned to avoid applying for a buffer from the heap.

Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
@crafcat7
Copy link
Contributor Author

@crafcat7 please fix the conflict.

Done

config LIBC_MAX_PATHBUFFER
int "Maximum size of a temporary file path buffer array"
range 0 32
default 2
Copy link
Contributor

Choose a reason for hiding this comment

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

So this should either be 0 or only be set if fs_lock is enabled.
Otherwise we're wasting 512 bytes of RAM.

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Aug 21, 2024

Choose a reason for hiding this comment

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

actually, we don't need do this optimization manually, the linker is enough smart to remove the unused code and data from the final image automatically if nobody calls lib_[get|put]_pathbuffer.

Summary:
  Adjust code logic

Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 271f765 into apache:master Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants