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 2022/08/09 07:56:58 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request, #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
xiaoxiang781216 opened a new pull request, #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816
## Summary
since l2cc_invalidate_all already do the protection
## Impact
## 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 closed pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
URL: https://github.com/apache/nuttx/pull/6816
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1211063834
> I do not have a strong opinion on this. The PR description states "since l2cc_invalidate_all already do the protection" ("callee" direction), but discussion goes into "caller" direction, so that is a bit confusing. I think that critical section here is done to ensure that invalidation sequence is not interrupted, so removing of a critical section allows an interruption, so calling it from a critical section becomes a caller responsibility. I think this is what @hartmannathan is talking about and it sounds reasonable to at least specify this in comment
The execution can be interrupted between cp15 and l2cc, if you go through the whole file, you will find that all other functions don't hold the critical section at all.
If you look at the assemble code, all cp15 cache operation can work without critical section too, only l2cc need to protect.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1218034816
> > @xiaoxiang781216 @pkarashchenko I studied this quite a bit. I see that l2cc_invalidate_all() is doing its own critical section.
> > cp15_invalidate_dcache_all() (which is assembly code) is not doing any critical section. It runs in a loop and clears one way at a time. I can't seem to find information from ARM that explains whether interrupts should be disabled when invalidating the D Cache. Is it safe to clear D Cache with interrupts enabled? If cp15_invalidate_dcache_all() is interrupted, and the interrupt reads or writes data that happens to be in the D Cache, will we get corruption?
> > I don't know the answer to above question.
> > If you are sure that this change is safe, then I'm OK with it.
>
> The loop only write not modify(read then write) CP15 register, so it's safe without the critical section.
@xiaoxiang781216 What about the contents of the cache itself?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1210919929
I do not have a strong opinion on this. The PR description states "since l2cc_invalidate_all already do the protection" ("callee" direction), but discussion goes into "caller" direction, so that is a bit confusing.
I think that critical section here is done to ensure that invalidation sequence is not interrupted, so removing of a critical section allows an interruption, so calling it from a critical section becomes a caller responsibility. I think this is what @hartmannathan is talking about and it sounds reasonable to at least specify this in comment
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1217887920
> @hartmannathan do you want to take a look?
@pkarashchenko yes, looking now...
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1218053935
> > > @xiaoxiang781216 @pkarashchenko I studied this quite a bit. I see that l2cc_invalidate_all() is doing its own critical section.
> > > cp15_invalidate_dcache_all() (which is assembly code) is not doing any critical section. It runs in a loop and clears one way at a time. I can't seem to find information from ARM that explains whether interrupts should be disabled when invalidating the D Cache. Is it safe to clear D Cache with interrupts enabled? If cp15_invalidate_dcache_all() is interrupted, and the interrupt reads or writes data that happens to be in the D Cache, will we get corruption?
> > > I don't know the answer to above question.
> > > If you are sure that this change is safe, then I'm OK with it.
> >
> >
> > The loop only write not modify(read then write) CP15 register, so it's safe without the critical section.
>
> @xiaoxiang781216 What about the contents of the cache itself?
One cache line will invalidate(discard) in each loop.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1214438878
> > But there is a note in function description:
> >
> > ```
> > * NOTE: This function forces L1 and L2 cache operations to be atomic
> > * by disabling interrupts.
> > ```
>
> @pkarashchenko The wrong comment is removed.
If this is well covered by testing on your side, then I do not have any objections.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1218038983
> I mean is it safe if any other CP15 cache access occurs after CP15 invalidate, but before L2CC invalidate?
Or during the middle of CP15 invalidate?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1211752994
If this is a requirement, all other cache function should be protected too, I think.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1214413121
> But there is a note in function description:
>
> ```
> * NOTE: This function forces L1 and L2 cache operations to be atomic
> * by disabling interrupts.
> ```
The wrong comment is removed.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1218066850
> I also do not know the correct answer. I'm just thinking not from a read-modify-store perspective, but from a cache operation perspective. I mean is it safe if any other CP15 cache access occurs after CP15 invalidate, but before L2CC invalidate? Can we lose memory data integrity at this point?
It isn't the responsibility of cache layer, but the caller should take care. We do cache operation just because we want to sync with hardware which has DMA capability, not another thread. The critical section can't do any protection in this case.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#discussion_r942080300
##########
arch/arm/src/armv7-r/arm_cache.c:
##########
@@ -81,14 +81,8 @@ void up_invalidate_dcache(uintptr_t start, uintptr_t end)
void up_invalidate_dcache_all(void)
{
-#ifdef CONFIG_ARCH_L2CACHE
Review Comment:
why `#ifdef CONFIG_ARCH_L2CACHE` is removed?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1211649428
But there is a note in function description:
```
* NOTE: This function forces L1 and L2 cache operations to be atomic
* by disabling interrupts.
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1209397036
Here is the reason:
1. All l2cc_xxx hold the critical section internally
2. All other up_xxx functions in the same file doesn't hold the critical section
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#discussion_r942217804
##########
arch/arm/src/armv7-r/arm_cache.c:
##########
@@ -81,14 +81,8 @@ void up_invalidate_dcache(uintptr_t start, uintptr_t end)
void up_invalidate_dcache_all(void)
{
-#ifdef CONFIG_ARCH_L2CACHE
Review Comment:
Because if CONFIG_ARCH_L2CACHE isn't defined, all functions become empty macros:
https://github.com/apache/incubator-nuttx/blob/master/arch/arm/src/armv7-a/l2cc.h#L232-L240
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1218022576
> @xiaoxiang781216 @pkarashchenko I studied this quite a bit. I see that l2cc_invalidate_all() is doing its own critical section.
>
> cp15_invalidate_dcache_all() (which is assembly code) is not doing any critical section. It runs in a loop and clears one way at a time. I can't seem to find information from ARM that explains whether interrupts should be disabled when invalidating the D Cache. Is it safe to clear D Cache with interrupts enabled? If cp15_invalidate_dcache_all() is interrupted, and the interrupt reads or writes data that happens to be in the D Cache, will we get corruption?
>
> I don't know the answer to above question.
>
> If you are sure that this change is safe, then I'm OK with it.
The loop only write not modify(read then write) CP15 register, so it's safe without the critical section.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1218036503
I also do not know the correct answer. I'm just thinking not from a read-modify-store perspective, but from a cache operation perspective. I mean is it safe if any other CP15 cache access occurs after CP15 invalidate, but before L2CC invalidate? Can we lose memory data integrity at this point?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1217546157
@hartmannathan do you want to take a look?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6816: arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6816:
URL: https://github.com/apache/incubator-nuttx/pull/6816#issuecomment-1215324154
Sure.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org