You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/08/20 07:09:37 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #1611: libc/stdio: Allocate file_struct dynamically
xiaoxiang781216 opened a new pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611
## Summary
1.Reduce the default size of task_group_s(~512B each task)
2.Scale better between simple and complex application
## Impact
In our system(10 task/kthread) save 4KB RAM
## Testing
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-677436957
@xiaoxiang781216 - I think you hit the point were changes to apps and nuttx and docs need to be atomic. What is youe plan here to get this built, tested and verified?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691089781
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] btashton commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691132135
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-678150846
> > > 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).
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] liuguo09 merged pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
liuguo09 merged pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691096139
> > I would like to test this in its entirety once it does pass.
>
> @v01d Unfortunately no.
>
> Since we could not get a clean CI pass on it. it should not have been merged.
This PR can't pass CI just because it need work with https://github.com/apache/incubator-nuttx-apps/pull/368. But it is an CI issue, not the PR problem. The community already have an agreement that the reviewer could focus on the chnage but ignore CI complain before the build system can cover this case. So CI failure isn't a good reason to block PR like this one.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] v01d commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691059442
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] v01d commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691059442
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691077050
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691207971
> > I am fine with what @Ouss4 suggested. In that case, if the build blows up, do we revert the PR or just keep iterating until we get the master working building and working again?
>
> If it's an easy fix, we don't need to revert, we just provide another PR to address the issue. Yes that means we'll have a small window where things are broken, but that shouldn't stay for too long. If we can't see an immediate solution, we'll have to revert and look again at the problem at hands.
@Ouss4 - thank you for outlining this. It seems workable. My latest practice for uptake-ing master
I do this for both 'apps' and 'nuttx' - this
```
git fetch nuttx
git checkout master-LAST
git reset --hard nuttx/master
git checkout master
git pull master nuttx master
```
It allows me a quick exit if the master is broken.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] liuguo09 merged pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
liuguo09 merged pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-678150846
> > > 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.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] v01d commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691138675
I agree that until a better solution is found to interdependent PRs we do as @Ouss4 describes.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-677452528
> > > @xiaoxiang781216 - I think you hit the point were changes to apps and nuttx and docs need to be atomic. What is your plan here to get this built, tested and verified?
> >
> >
> > @btashton has idea to tie the related patch in apps and nuttx by some special tag.
>
> @xiaoxiang781216 is it working?
It should work in theory.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691096139
> > I would like to test this in its entirety once it does pass.
>
> @v01d Unfortunately no.
>
> Since we could not get a clean CI pass on it. it should not have been merged.
This PR can't pass CI just because it need work with https://github.com/apache/incubator-nuttx-apps/pull/368. But it is an CI issue, not the PR problem. The community already have an agreement that the reviewer could focus on the change but ignore CI complain before the build system can cover this case. So CI failure isn't a good reason to block PR like this one.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment 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. And more, I have ping @davids5 several time, but he never give a response.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691096139
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691175358
@btashton I am sorry for mentioning sub modules. I know an appreciate the time you all have put in getting the CI as far as it is. It is that kast 1/10 mile thing.... I fully recognize that we all have day jobs. So when it can happen I am sure you can and will make it happen. I gave 3 options as quick fixes I not trying to offend here. My thinking is now that we have all had some real github experience it may very well be worth updating the work flow and considering other ways to use the tools.
I.E. Move the testing in tree. Squashing the PRs. Of course only after there is an agreement.
I am fine with what @Ouss4 suggested. In that case, if the build blows up, do we revert the PR or just keep iterating until we get the master working building and working again? My concern is the same as yours, we are busy, and when master breaks it puts an immediate pressure on all of us. Building the branches together will remove that immediacy.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-677436957
@xiaoxiang781216 - I think you hit the point were changes to apps and nuttx and docs need to be atomic. What is your plan here to get this built, tested and verified?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691492899
> @xiaoxiang781216 This PR has potential issues that needed to be tested. Expecting us to piece together the parts and test it far from best practices. You can a_ Add the links and keep them updated here. 1) make a repo and tie the commits together with submodules or 3) add a shell script to pull the branches. Then have the CI look for it and use it to test.
With the huge change like this, we normally build and test carefully before send PR. But since there are many boards with the different configuration which are hard to cover all corner cases in our local build testing. So I always sit on my desk to fix any poentially build break. Actually, @liuguo09 and @btashton take a lot of time to create the automation build system from scratch. Yes, the build system isn't perfect yet, but it can catch most build issue quickly after merge. If you have any better idea, please make a contribution and show me your code or PR, instead complain.
Let's come back the build break inroduced by this PR(I have fixed it here: https://github.com/apache/incubator-nuttx/pull/1755).
Why this error happen? Because you block this PR for more than three weeks and new config is added during this period!!! I answer all your concern quickly, but after that you stop response. I respect each comment made by reviewer, so please respect my reply too.
>
> BOTTOM Line is it needed BUILT to tested.
>
> Are you up for reverting it?
Why revert my change? Do you have hit any problem with it?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-677436957
@xiaoxiang781216 - I think you hit the point were changes to apps and nuttx and docs need to be atomic. What is you plan here to get this built, tested and verified?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-678137091
> > 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?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691183952
> I am fine with what @Ouss4 suggested. In that case, if the build blows up, do we revert the PR or just keep iterating until we get the master working building and working again?
If it's an easy fix, we don't need to revert, we just provide another PR to address the issue. Yes that means we'll have a small window where things are broken, but that shouldn't stay for too long. If we can't see an immediate solution, we'll have to revert and look again at the problem at hands.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by 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
[GitHub] [incubator-nuttx] v01d commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691100855
@xiaoxiang781216 my question was due to this comment:
> I would like to test this in its entirety once it does pass.
not due to the CI failure. I understand that CI wouldn't pass in this case.
Regarding pending unanswered/reviews: we should be mindful that people may not respond in time since no one in here is in the obligation to do so and may have reasons for not being available. Of course if that happens, to avoid delaying important fixes we should seek review from someone else.
@davids5 can you have a look at the PR linked by @xiaoxiang781216 ? I looked at it but I don't really know about this part of NuttX so I can't comment.
But the important thing is that anyone who merges should ensure that all comments have been addressed and ask for an "LGTM" before merging. Also, I think we should get into the habit of using the "approve PR" or "request changes" from GitHub so that this becomes clear.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691510278
Sure, let's wait your report.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691089781
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-678134119
> 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.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] liuguo09 merged pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
liuguo09 merged pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691096139
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] v01d commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691059442
@davids5 where your concerns addresses here?
This was just merged and it doesn't seem a LGTM has been given
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691508015
@xiaoxiang781216 - I will test it next week.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691077050
>I would like to test this in its entirety once it does pass.
@v01d Unfortunately no.
Since we could not get a clean CI pass on it. it should not have been merged.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-692041354
Comparing
Master before this PR 1c488eb864ff657474f97c342ec5c8fcd2bcb7fd B
to Master with this PR 34b34e2d4519956f9a61f5985aca29947c4b8b23 A
@xiaoxiang781216 Sorry for wanting to be so cautions, over all this looks quite good!
The acid test would be to see how over time, the fragmentation is effected. I do not have time to do that now. Did you do any testing like that?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691104888
Sure,
> @xiaoxiang781216 my question was due to this comment:
>
> > I would like to test this in its entirety once it does pass.
>
> not due to the CI failure. I understand that CI wouldn't pass in this case.
>
> Regarding pending unanswered/reviews: we should be mindful that people may not respond in time since no one in here is in the obligation to do so and may have reasons for not being available. Of course if that happens, to avoid delaying important fixes we should seek review from someone else.
Yes, I normally ping reviewer if he/she doesn't response longer than one week.
> @davids5 can you have a look at the PR linked by @xiaoxiang781216 ? I looked at it but I don't really know about this part of NuttX so I can't comment.
>
> But the important thing is that anyone who merges should ensure that all comments have been addressed and ask for an "LGTM" before merging. Also, I think we should get into the habit of using the "approve PR" or "request changes" from GitHub so that this becomes clear.
Yes, it's a very good suggestion to follow.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691492899
> @xiaoxiang781216 This PR has potential issues that needed to be tested. Expecting us to piece together the parts and test it far from best practices. You can a_ Add the links and keep them updated here. 1) make a repo and tie the commits together with submodules or 3) add a shell script to pull the branches. Then have the CI look for it and use it to test.
With the huge change like this, we normally build and test carefully before send PR. But since there are many boards with the different configuration which are hard to cover all corner cases in our local build testing. So I always sit on my desk to fix any poentially build break.
Actually, @liuguo09 and @btashton take a lot of time to create the automation build system from scratch. Yes, the build system isn't perfect yet, but it can catch most build issue quickly after merge. If you have any better idea, please make a contribution and show me your code or PR, instead complain.
Let's come back the build break inroduced by this PR(I have fixed it here: https://github.com/apache/incubator-nuttx/pull/1755).
Why this error happen? Because you block this PR for more than three weeks and new config is added during this period!!! I answer all your concern quickly, but after that you stop response. I respect each comment made by reviewer, so please respect my reply too.
>
> BOTTOM Line is it needed BUILT to tested.
>
> Are you up for reverting it?
Why revert my change? Do you have hit any problem with it? I am here to prepare fix any issue you report.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] v01d commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691059442
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691492899
> @xiaoxiang781216 This PR has potential issues that needed to be tested. Expecting us to piece together the parts and test it far from best practices. You can a_ Add the links and keep them updated here. 1) make a repo and tie the commits together with submodules or 3) add a shell script to pull the branches. Then have the CI look for it and use it to test.
With the huge change like this, we normally build and test carefully before send PR. But since there are many boards with the different configuration which are hard to cover all corner cases in our local build testing. So I always sit on my desk to fix any poentially build break.
Actually, @liuguo09 and @btashton take a lot of time to create the automation build system from scratch. Yes, the build system isn't perfect yet, but it can catch most build issue quickly after merge. If you have any better idea, please make a contribution and show me your code or PR, instead complain.
Let's come back the build break inroduced by this PR(I have fixed it here: https://github.com/apache/incubator-nuttx/pull/1755).
Why this error happen? Because you block this PR for more than three weeks and new config is added during this period!!! I answer all your concern quickly, but after that you stop response. I respect each comment made by reviewer, so please respect my reply too.
>
> BOTTOM Line is it needed BUILT to tested.
>
> Are you up for reverting it?
Why revert my change? Do you have hit any problem with it?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] v01d commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691078773
@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
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691123628
@liuguo09 it is **ASK** for review's LGTM. Not just drop a comment. and commit it because the boss told you to.
@xiaoxiang781216 This PR has potential issues that needed to be tested. Expecting us to piece together the parts and test it far from best practices. You can a_ Add the links and keep them updated here. 1) make a repo and tie the commits together with submodules or 3) add a shell script to pull the branches. Then have the CI look for it and use it to test.
BOTTOM Line is it needed BUILT to tested.
Are you up for reverting it?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] btashton commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691132135
@davids5 please stop it with the submodules stuff and attacks it's not productive. Some of us have put a lot of time into getting the CI stuff wired together and these offhanded comments are getting really old.
There is a plan to improve the linking of related PRs but it's not something I personally want to do at the moment having spent way more time doing CI work than OS work in the last few weeks.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] liuguo09 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
liuguo09 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691091199
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-692189356
We have a pressure testing(run 500~ OTA, the peek free memory below 10K) daily, no the regression report so far.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691127570
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] btashton commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691132135
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] liuguo09 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
liuguo09 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691091199
@v01d Thanks for reminding. I'll give comment before mergeing in future.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] btashton commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-678236018
> > 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.
This is correct, I started doing it in the CI yaml, but it got too complex so I wanted to move it and some of the branch logic into a real action that can be shared. I have not finished that work.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691127570
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] liuguo09 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
liuguo09 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691091199
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment 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 #997. 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. And more, I have ping @davids5 several time, but he never give a response.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691077050
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-677434615
@v01d there is a documentation change here for PR #1501
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] liuguo09 merged pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
liuguo09 merged pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-677449294
> @xiaoxiang781216 - I think you hit the point were changes to apps and nuttx and docs need to be atomic. What is your plan here to get this built, tested and verified?
@btashton has idea to tie the related patch in apps and nuttx by some special tag.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691127570
What we've been doing so far when there is a PR that depends on another, is to merge the easiest to review (most of the times the apps/ one) and re-start the checks. Still not ideal, but gets us going.
Also please note that once merged the CI is run again. I see Xiang's commit passed all checks (expect one macOs which again is not related to the change.)
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] liuguo09 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
liuguo09 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-691091199
@v01d Thanks for reminding. I'll give comment before mergeing in future.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1611:
URL: https://github.com/apache/incubator-nuttx/pull/1611#issuecomment-677450742
> > @xiaoxiang781216 - I think you hit the point were changes to apps and nuttx and docs need to be atomic. What is your plan here to get this built, tested and verified?
>
> @btashton has idea to tie the related patch in apps and nuttx by some special tag.
@xiaoxiang781216 is it working?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1611: libc/stdio: Allocate file_struct dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment 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 #997. 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 meet it in our device), we should fix the regression as soon as possible. But the disscussion stop again for more than two months. And more, I have ping @davids5 several time, but he never give a response.
----------------------------------------------------------------
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