| Message ID | 1624360271-17525-2-git-send-email-huangguangbin2@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hns3: add support for TX push | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: marcan@marcan.st andreyknvl@gmail.com linus.walleij@linaro.org wangxiongfeng2@huawei.com suzuki.poulose@arm.com wangzhou1@hisilicon.com ardb@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 2 this patch: 2 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | fail | ERROR: Macros with complex values should be enclosed in parentheses |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Tue, Jun 22, 2021 at 07:11:09PM +0800, Guangbin Huang wrote: > From: Xiongfeng Wang <wangxiongfeng2@huawei.com> > > DGH prohibits merging memory accesses with Normal-NC or Device-GRE > attributes before the hint instruction with any memory accesses > appearing after the hint instruction. Provide macros to expose it to the > arch code. Hmm. The architecture states: | DGH is a hint instruction. A DGH instruction is not expected to be | performance optimal to merge memory accesses with Normal Non-cacheable | or Device-GRE attributes appearing in program order before the hint | instruction with any memory accesses appearing after the hint instruction | into a single memory transaction on an interconnect. which doesn't make a whole lot of sense to me, in all honesty. > Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> > Signed-off-by: Cheng Jian <cj.chengjian@huawei.com> > Signed-off-by: Yufeng Mo <moyufeng@huawei.com> > --- > arch/arm64/include/asm/assembler.h | 7 +++++++ > arch/arm64/include/asm/barrier.h | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h > index 8418c1bd8f04..d723899328bd 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -90,6 +90,13 @@ > .endm > > /* > + * Data gathering hint > + */ > + .macro dgh > + hint #6 > + .endm > + > +/* > * RAS Error Synchronization barrier > */ > .macro esb > diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h > index 451e11e5fd23..02e1735706d2 100644 > --- a/arch/arm64/include/asm/barrier.h > +++ b/arch/arm64/include/asm/barrier.h > @@ -22,6 +22,7 @@ > #define dmb(opt) asm volatile("dmb " #opt : : : "memory") > #define dsb(opt) asm volatile("dsb " #opt : : : "memory") > > +#define dgh() asm volatile("hint #6" : : : "memory") Although I'm fine with this in arm64, I don't think this is the interface which drivers should be using. Instead, once we know what this instruction is supposed to do, we should look at exposing it as part of the I/O barriers and providing a NOP implementation for other architectures. That way, drivers can use it without having to have the #ifdef CONFIG_ARM64 stuff that you have in the later patches here. Will
On Tue, Jun 22, 2021 at 01:16:31PM +0100, Will Deacon wrote: > On Tue, Jun 22, 2021 at 07:11:09PM +0800, Guangbin Huang wrote: > > From: Xiongfeng Wang <wangxiongfeng2@huawei.com> > > > > DGH prohibits merging memory accesses with Normal-NC or Device-GRE > > attributes before the hint instruction with any memory accesses > > appearing after the hint instruction. Provide macros to expose it to the > > arch code. > > Hmm. > > The architecture states: > > | DGH is a hint instruction. A DGH instruction is not expected to be > | performance optimal to merge memory accesses with Normal Non-cacheable > | or Device-GRE attributes appearing in program order before the hint > | instruction with any memory accesses appearing after the hint instruction > | into a single memory transaction on an interconnect. > > which doesn't make a whole lot of sense to me, in all honesty. I think there are some missing words, and this was supposed to say something like: | DGH is a hint instruction. A DGH instruction *indicates that it* is | not expected to be performance optimal to merge memory accesses with | Normal Non-cacheable or Device-GRE attributes appearing in program | order before the hint instruction with any memory accesses appearing | after the hint instruction into a single memory transaction on an | interconnect. ... i.e. it's a hint to the CPU to avoid merging accesses which are either side of the DGH, so that the prior accesses don't get indefinitely delayed waiting to be merged. I'll try to get the documentation fixed, since as-is the wording does not make sense. Thanks, Mark.
On 2021/6/22 20:16, Will Deacon wrote: > On Tue, Jun 22, 2021 at 07:11:09PM +0800, Guangbin Huang wrote: >> From: Xiongfeng Wang <wangxiongfeng2@huawei.com> >> >> DGH prohibits merging memory accesses with Normal-NC or Device-GRE >> attributes before the hint instruction with any memory accesses >> appearing after the hint instruction. Provide macros to expose it to the >> arch code. > > Hmm. > > The architecture states: > > | DGH is a hint instruction. A DGH instruction is not expected to be > | performance optimal to merge memory accesses with Normal Non-cacheable > | or Device-GRE attributes appearing in program order before the hint > | instruction with any memory accesses appearing after the hint instruction > | into a single memory transaction on an interconnect. > > which doesn't make a whole lot of sense to me, in all honesty. > Thanks for your review and modification of commit log. >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> >> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com> >> Signed-off-by: Yufeng Mo <moyufeng@huawei.com> >> --- >> arch/arm64/include/asm/assembler.h | 7 +++++++ >> arch/arm64/include/asm/barrier.h | 1 + >> 2 files changed, 8 insertions(+) >> >> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h >> index 8418c1bd8f04..d723899328bd 100644 >> --- a/arch/arm64/include/asm/assembler.h >> +++ b/arch/arm64/include/asm/assembler.h >> @@ -90,6 +90,13 @@ >> .endm >> >> /* >> + * Data gathering hint >> + */ >> + .macro dgh >> + hint #6 >> + .endm >> + >> +/* >> * RAS Error Synchronization barrier >> */ >> .macro esb >> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h >> index 451e11e5fd23..02e1735706d2 100644 >> --- a/arch/arm64/include/asm/barrier.h >> +++ b/arch/arm64/include/asm/barrier.h >> @@ -22,6 +22,7 @@ >> #define dmb(opt) asm volatile("dmb " #opt : : : "memory") >> #define dsb(opt) asm volatile("dsb " #opt : : : "memory") >> >> +#define dgh() asm volatile("hint #6" : : : "memory") > > Although I'm fine with this in arm64, I don't think this is the interface > which drivers should be using. Instead, once we know what this instruction > is supposed to do, we should look at exposing it as part of the I/O barriers > and providing a NOP implementation for other architectures. That way, > drivers can use it without having to have the #ifdef CONFIG_ARM64 stuff that > you have in the later patches here. > > Will > . > Ok, thanks, we will try to implement a new I/O barriers interface as your opinion and repost a new version after we test ok.
On 2021/6/22 20:32, Mark Rutland wrote: > On Tue, Jun 22, 2021 at 01:16:31PM +0100, Will Deacon wrote: >> On Tue, Jun 22, 2021 at 07:11:09PM +0800, Guangbin Huang wrote: >>> From: Xiongfeng Wang <wangxiongfeng2@huawei.com> >>> >>> DGH prohibits merging memory accesses with Normal-NC or Device-GRE >>> attributes before the hint instruction with any memory accesses >>> appearing after the hint instruction. Provide macros to expose it to the >>> arch code. >> >> Hmm. >> >> The architecture states: >> >> | DGH is a hint instruction. A DGH instruction is not expected to be >> | performance optimal to merge memory accesses with Normal Non-cacheable >> | or Device-GRE attributes appearing in program order before the hint >> | instruction with any memory accesses appearing after the hint instruction >> | into a single memory transaction on an interconnect. >> >> which doesn't make a whole lot of sense to me, in all honesty. > > I think there are some missing words, and this was supposed to say > something like: > > | DGH is a hint instruction. A DGH instruction *indicates that it* is > | not expected to be performance optimal to merge memory accesses with > | Normal Non-cacheable or Device-GRE attributes appearing in program > | order before the hint instruction with any memory accesses appearing > | after the hint instruction into a single memory transaction on an > | interconnect. > > ... i.e. it's a hint to the CPU to avoid merging accesses which are > either side of the DGH, so that the prior accesses don't get > indefinitely delayed waiting to be merged. > > I'll try to get the documentation fixed, since as-is the wording does > not make sense. > > Thanks, > Mark. > . > Thanks very much, we will fix the documentation. Thanks, Guangbin, .
Hi Will, On 2021/6/22 20:16, Will Deacon wrote: > On Tue, Jun 22, 2021 at 07:11:09PM +0800, Guangbin Huang wrote: >> From: Xiongfeng Wang <wangxiongfeng2@huawei.com> >> >> DGH prohibits merging memory accesses with Normal-NC or Device-GRE >> attributes before the hint instruction with any memory accesses >> appearing after the hint instruction. Provide macros to expose it to the >> arch code. > > Hmm. > > The architecture states: > > | DGH is a hint instruction. A DGH instruction is not expected to be > | performance optimal to merge memory accesses with Normal Non-cacheable > | or Device-GRE attributes appearing in program order before the hint > | instruction with any memory accesses appearing after the hint instruction > | into a single memory transaction on an interconnect. > > which doesn't make a whole lot of sense to me, in all honesty. > >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> >> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com> >> Signed-off-by: Yufeng Mo <moyufeng@huawei.com> >> --- >> arch/arm64/include/asm/assembler.h | 7 +++++++ >> arch/arm64/include/asm/barrier.h | 1 + >> 2 files changed, 8 insertions(+) >> >> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h >> index 8418c1bd8f04..d723899328bd 100644 >> --- a/arch/arm64/include/asm/assembler.h >> +++ b/arch/arm64/include/asm/assembler.h >> @@ -90,6 +90,13 @@ >> .endm >> >> /* >> + * Data gathering hint >> + */ >> + .macro dgh >> + hint #6 >> + .endm >> + >> +/* >> * RAS Error Synchronization barrier >> */ >> .macro esb >> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h >> index 451e11e5fd23..02e1735706d2 100644 >> --- a/arch/arm64/include/asm/barrier.h >> +++ b/arch/arm64/include/asm/barrier.h >> @@ -22,6 +22,7 @@ >> #define dmb(opt) asm volatile("dmb " #opt : : : "memory") >> #define dsb(opt) asm volatile("dsb " #opt : : : "memory") >> >> +#define dgh() asm volatile("hint #6" : : : "memory") > > Although I'm fine with this in arm64, I don't think this is the interface > which drivers should be using. Instead, once we know what this instruction > is supposed to do, we should look at exposing it as part of the I/O barriers > and providing a NOP implementation for other architectures. That way, > drivers can use it without having to have the #ifdef CONFIG_ARM64 stuff that > you have in the later patches here. How about we adding a interface called flush_wc_writeX(), which can be used to flush the write-combined buffers to the device immediately. I found it has been disscussed in the below link, but it is unnessary in their situation. https://patchwork.ozlabs.org/project/netdev/patch/20200102180830.66676-3-liran.alon@oracle.com/ Thanks, Xiongfeng > > Will > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > . >
Hi, On 2021/6/29 19:11, Xiongfeng Wang wrote: > Hi Will, > > On 2021/6/22 20:16, Will Deacon wrote: >> On Tue, Jun 22, 2021 at 07:11:09PM +0800, Guangbin Huang wrote: >>> From: Xiongfeng Wang <wangxiongfeng2@huawei.com> >>> >>> DGH prohibits merging memory accesses with Normal-NC or Device-GRE >>> attributes before the hint instruction with any memory accesses >>> appearing after the hint instruction. Provide macros to expose it to the >>> arch code. >> >> Hmm. >> >> The architecture states: >> >> | DGH is a hint instruction. A DGH instruction is not expected to be >> | performance optimal to merge memory accesses with Normal Non-cacheable >> | or Device-GRE attributes appearing in program order before the hint >> | instruction with any memory accesses appearing after the hint instruction >> | into a single memory transaction on an interconnect. >> >> which doesn't make a whole lot of sense to me, in all honesty. >> >>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> >>> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com> >>> Signed-off-by: Yufeng Mo <moyufeng@huawei.com> >>> --- >>> arch/arm64/include/asm/assembler.h | 7 +++++++ >>> arch/arm64/include/asm/barrier.h | 1 + >>> 2 files changed, 8 insertions(+) >>> >>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h >>> index 8418c1bd8f04..d723899328bd 100644 >>> --- a/arch/arm64/include/asm/assembler.h >>> +++ b/arch/arm64/include/asm/assembler.h >>> @@ -90,6 +90,13 @@ >>> .endm >>> >>> /* >>> + * Data gathering hint >>> + */ >>> + .macro dgh >>> + hint #6 >>> + .endm >>> + >>> +/* >>> * RAS Error Synchronization barrier >>> */ >>> .macro esb >>> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h >>> index 451e11e5fd23..02e1735706d2 100644 >>> --- a/arch/arm64/include/asm/barrier.h >>> +++ b/arch/arm64/include/asm/barrier.h >>> @@ -22,6 +22,7 @@ >>> #define dmb(opt) asm volatile("dmb " #opt : : : "memory") >>> #define dsb(opt) asm volatile("dsb " #opt : : : "memory") >>> >>> +#define dgh() asm volatile("hint #6" : : : "memory") >> >> Although I'm fine with this in arm64, I don't think this is the interface >> which drivers should be using. Instead, once we know what this instruction >> is supposed to do, we should look at exposing it as part of the I/O barriers >> and providing a NOP implementation for other architectures. That way, >> drivers can use it without having to have the #ifdef CONFIG_ARM64 stuff that >> you have in the later patches here. > > How about we adding a interface called flush_wc_writeX(), which can be used to > flush the write-combined buffers to the device immediately. > I found it has been disscussed in the below link, but it is unnessary in their > situation. > https://patchwork.ozlabs.org/project/netdev/patch/20200102180830.66676-3-liran.alon@oracle.com/ Do you have some suggestions on this problem ? How about we adding an interface called flush_wc_writeX() ? Thanks, Xiongfeng > > Thanks, > Xiongfeng > >> >> Will >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> . >>
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index 8418c1bd8f04..d723899328bd 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -90,6 +90,13 @@ .endm /* + * Data gathering hint + */ + .macro dgh + hint #6 + .endm + +/* * RAS Error Synchronization barrier */ .macro esb diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h index 451e11e5fd23..02e1735706d2 100644 --- a/arch/arm64/include/asm/barrier.h +++ b/arch/arm64/include/asm/barrier.h @@ -22,6 +22,7 @@ #define dmb(opt) asm volatile("dmb " #opt : : : "memory") #define dsb(opt) asm volatile("dsb " #opt : : : "memory") +#define dgh() asm volatile("hint #6" : : : "memory") #define psb_csync() asm volatile("hint #17" : : : "memory") #define tsb_csync() asm volatile("hint #18" : : : "memory") #define csdb() asm volatile("hint #20" : : : "memory")