[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically

[GitBox <gi...@apache.org>]

xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691089781


   > @liuguo09 please consider comments in open PRs carefully before merging. This caught my atention since I saw many PRs merged without much further discussion.
   > @davids5 feel free to revert if you prefer to do so
   
   @v01d I have reply @davids5 all concern here for more than three weeks:
   > > > > It should work in theory.
   > > > 
   > > > 
   > > > It's already in place for branches, @btashton set it up, we use it for releases.
   > > > An extra effort is needed to make it work with PRs, I guess @btashton already has a draft somewhere.
   > > 
   > > 
   > > Thanks @Ouss4! Good to know. Can we get it to pass CI?
   > > I would like to test this in its entirety once it does pass.
   > > @xiaoxiang781216 Give that the static allocation was not subject to fragmentation, would you explain the potential impact with this change to a long running system?
   > 
   > If the system start/stop task frequently, the fragmentation may be a problem, but there is already many dynamic allocation to start a task:
   > 1.task_group_s
   > 2.task_tcb_s
   > 3.environ
   > 4.stack
   > so the change don't degrade the situation too much. Also, if the fragementation is a problem, it's better to utilize a global file_struct pool instead per task since it's more memory efficient and scalable(simple task v.s. complex task).
   > And it's a simple task to add the pool management on top of the current implementation.
   
   Do we have any requirement on the reviewer to reply the disscussion timely? Otherwise, how we handle some reviewer stop the disscussion like this? Here another example: https://github.com/apache/incubator-nuttx/pull/1369
   That patch fix a fatal error introduced by PR #986. Devices can't boot into shell if the config meet the follow condition:
   1.Enable CONFIG_RTC
   2.Enable CONFIG_LIBC_LOCALTIME
   3.But don't setup rtcb->adj_stack_ptr correctly
   The combination is very common, we should fix the regression as soon as possible. But the disscussion stop again for more than two months.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org