Re: [Intel-gfx] [RFC DO NOT MERGE] treewide: use __xchg in most obvious places
On Tue, Jan 10, 2023 at 01:46:37PM +0100, Andrzej Hajda wrote: > On 10.01.2023 12:07, Andy Shevchenko wrote: > > On Tue, Jan 10, 2023 at 11:53:06AM +0100, Andrzej Hajda wrote: ... > > > + return __xchg(_chain->p_prod_elem, > > > + (void *)(((u8 *)p_chain->p_prod_elem) + > > > p_chain->elem_size)); > > > > Wondering if you still need a (void *) casting after the change. Ditto for > > the > > rest of similar cases. > > IMHO it is not needed also before the change and IIRC gcc has an extension > which allows to drop (u8 *) cast as well [1]. I guess you can drop at least the former one. > [1]: https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html ... > > Btw, is it done by coccinelle? If no, why not providing the script? > > Yes I have used cocci. My cocci skills are far from perfect, so I did not > want to share my dirty code, but this is nothing secret: Thank you! It's not about secrecy, it's about automation / error proofness. -- With Best Regards, Andy Shevchenko
Re: [Intel-gfx] [RFC DO NOT MERGE] treewide: use __xchg in most obvious places
On 10.01.2023 12:07, Andy Shevchenko wrote:
On Tue, Jan 10, 2023 at 11:53:06AM +0100, Andrzej Hajda wrote:
This patch tries to show usability of __xchg helper.
It is not intended to be merged, but I can convert
it to proper patchset if necessary.
There are many more places where __xchg can be used.
This demo shows the most spectacular cases IMHO:
- previous value is returned from function,
- temporary variables are in use.
As a result readability is much better and diffstat is quite
nice, less local vars to look at.
In many cases whole body of functions is replaced
with __xchg(ptr, val), so as further refactoring the whole
function can be removed and __xchg can be called directly.
...
arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
struct pt_regs *regs)
{
- unsigned long orig_ret_vaddr;
-
- orig_ret_vaddr = regs->ARM_lr;
- /* Replace the return addr with trampoline addr */
- regs->ARM_lr = trampoline_vaddr;
- return orig_ret_vaddr;
+ return __xchg(>ARM_lr, trampoline_vaddr);
}
If it's not a callback, the entire function can be killed.
And this is a good example of the function usage.
OTOH, these places might have a side effect (if it's in deep CPU
handlers), means we need to do this carefully.
...
static inline void *qed_chain_produce(struct qed_chain *p_chain)
{
- void *p_ret = NULL, *p_prod_idx, *p_prod_page_idx;
+ void *p_prod_idx, *p_prod_page_idx;
if (is_chain_u16(p_chain)) {
if ((p_chain->u.chain16.prod_idx &
@@ -390,11 +391,8 @@ static inline void *qed_chain_produce(struct qed_chain
*p_chain)
p_chain->u.chain32.prod_idx++;
}
- p_ret = p_chain->p_prod_elem;
- p_chain->p_prod_elem = (void *)(((u8 *)p_chain->p_prod_elem) +
- p_chain->elem_size);
-
- return p_ret;
+ return __xchg(_chain->p_prod_elem,
+ (void *)(((u8 *)p_chain->p_prod_elem) +
p_chain->elem_size));
Wondering if you still need a (void *) casting after the change. Ditto for the
rest of similar cases.
IMHO it is not needed also before the change and IIRC gcc has an
extension which allows to drop (u8 *) cast as well [1].
[1]: https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
}
...
Btw, is it done by coccinelle? If no, why not providing the script?
Yes I have used cocci. My cocci skills are far from perfect, so I did
not want to share my dirty code, but this is nothing secret:
@r1@
expression x, v;
local idexpression p;
@@
- p = x;
- x = v;
- return p;
+ return __xchg(, v);
@depends on r1@
expression e;
@@
__xchg(
- &*e,
+ e,
...)
@depends on r1@
expression t;
@@
- if (t) {
+ if (t)
return __xchg(...);
- }
@depends on r1@
type t;
identifier p;
expression e;
@@
(
- t p;
|
- t p = e;
)
... when != p
Regards
Andrzej
Re: [RFC DO NOT MERGE] treewide: use __xchg in most obvious places
On Tue, Jan 10, 2023 at 11:53:06AM +0100, Andrzej Hajda wrote:
> This patch tries to show usability of __xchg helper.
> It is not intended to be merged, but I can convert
> it to proper patchset if necessary.
>
> There are many more places where __xchg can be used.
> This demo shows the most spectacular cases IMHO:
> - previous value is returned from function,
> - temporary variables are in use.
>
> As a result readability is much better and diffstat is quite
> nice, less local vars to look at.
> In many cases whole body of functions is replaced
> with __xchg(ptr, val), so as further refactoring the whole
> function can be removed and __xchg can be called directly.
...
> arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
> struct pt_regs *regs)
> {
> - unsigned long orig_ret_vaddr;
> -
> - orig_ret_vaddr = regs->ARM_lr;
> - /* Replace the return addr with trampoline addr */
> - regs->ARM_lr = trampoline_vaddr;
> - return orig_ret_vaddr;
> + return __xchg(>ARM_lr, trampoline_vaddr);
> }
If it's not a callback, the entire function can be killed.
And this is a good example of the function usage.
OTOH, these places might have a side effect (if it's in deep CPU
handlers), means we need to do this carefully.
...
> static inline void *qed_chain_produce(struct qed_chain *p_chain)
> {
> - void *p_ret = NULL, *p_prod_idx, *p_prod_page_idx;
> + void *p_prod_idx, *p_prod_page_idx;
>
> if (is_chain_u16(p_chain)) {
> if ((p_chain->u.chain16.prod_idx &
> @@ -390,11 +391,8 @@ static inline void *qed_chain_produce(struct qed_chain
> *p_chain)
> p_chain->u.chain32.prod_idx++;
> }
>
> - p_ret = p_chain->p_prod_elem;
> - p_chain->p_prod_elem = (void *)(((u8 *)p_chain->p_prod_elem) +
> - p_chain->elem_size);
> -
> - return p_ret;
> + return __xchg(_chain->p_prod_elem,
> + (void *)(((u8 *)p_chain->p_prod_elem) +
> p_chain->elem_size));
Wondering if you still need a (void *) casting after the change. Ditto for the
rest of similar cases.
> }
...
Btw, is it done by coccinelle? If no, why not providing the script?
--
With Best Regards,
Andy Shevchenko
[RFC DO NOT MERGE] treewide: use __xchg in most obvious places
This patch tries to show usability of __xchg helper.
It is not intended to be merged, but I can convert
it to proper patchset if necessary.
There are many more places where __xchg can be used.
This demo shows the most spectacular cases IMHO:
- previous value is returned from function,
- temporary variables are in use.
As a result readability is much better and diffstat is quite
nice, less local vars to look at.
In many cases whole body of functions is replaced
with __xchg(ptr, val), so as further refactoring the whole
function can be removed and __xchg can be called directly.
Signed-off-by: Andrzej Hajda
---
arch/arm/probes/uprobes/core.c| 8 ++--
arch/csky/kernel/probes/uprobes.c | 9 ++---
arch/mips/kernel/irq_txx9.c | 7 ++-
arch/mips/kernel/process.c| 8 +++-
arch/mips/kernel/uprobes.c| 10 ++
arch/powerpc/include/asm/kvm_ppc.h| 7 ++-
arch/powerpc/kernel/uprobes.c | 10 ++
arch/powerpc/mm/init_64.c | 7 ++-
arch/riscv/kernel/probes/uprobes.c| 9 ++---
arch/s390/kernel/uprobes.c| 7 ++-
arch/s390/kvm/interrupt.c | 6 ++
arch/sh/kernel/traps_32.c | 6 ++
.../accessibility/speakup/speakup_dectlk.c| 7 ++-
drivers/accessibility/speakup/speakup_soft.c | 7 ++-
drivers/block/drbd/drbd_receiver.c| 5 ++---
drivers/cdrom/cdrom.c | 7 ++-
drivers/gpu/drm/drm_atomic_uapi.c | 14 +++---
drivers/iommu/iova.c | 7 ++-
drivers/misc/ti-st/st_core.c | 10 +++---
drivers/mtd/nand/raw/qcom_nandc.c | 11 ---
drivers/net/ethernet/ibm/ehea/ehea_main.c | 11 +++
.../microchip/sparx5/sparx5_calendar.c| 10 --
drivers/net/usb/rtl8150.c | 9 +++--
drivers/net/wireless/intel/iwlwifi/pcie/rx.c | 8 +++-
.../net/wireless/marvell/mwifiex/sta_ioctl.c | 7 +++
drivers/scsi/device_handler/scsi_dh_alua.c| 8 +++-
drivers/scsi/lpfc/lpfc_sli.c | 7 ++-
drivers/staging/rtl8192e/rtllib_tx.c | 7 ++-
drivers/tty/hvc/hvc_iucv.c| 8 +++-
drivers/video/fbdev/sis/sis_main.c| 6 ++
drivers/xen/grant-table.c | 6 ++
fs/namespace.c| 6 ++
include/linux/ptr_ring.h | 7 ++-
include/linux/qed/qed_chain.h | 19 +++
io_uring/io_uring.c | 7 ++-
mm/kmsan/init.c | 7 ++-
mm/memcontrol.c | 8 ++--
net/mac80211/rc80211_minstrel_ht.c| 6 ++
sound/pci/asihpi/hpidebug.c | 8 +++-
.../selftests/bpf/progs/dummy_st_ops.c| 7 ++-
40 files changed, 99 insertions(+), 225 deletions(-)
diff --git a/arch/arm/probes/uprobes/core.c b/arch/arm/probes/uprobes/core.c
index f5f790c6e5f896..77ce8ae431376d 100644
--- a/arch/arm/probes/uprobes/core.c
+++ b/arch/arm/probes/uprobes/core.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
@@ -61,12 +62,7 @@ unsigned long
arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
struct pt_regs *regs)
{
- unsigned long orig_ret_vaddr;
-
- orig_ret_vaddr = regs->ARM_lr;
- /* Replace the return addr with trampoline addr */
- regs->ARM_lr = trampoline_vaddr;
- return orig_ret_vaddr;
+ return __xchg(>ARM_lr, trampoline_vaddr);
}
int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
diff --git a/arch/csky/kernel/probes/uprobes.c
b/arch/csky/kernel/probes/uprobes.c
index 2d31a12e46cfee..775fe88b5f0016 100644
--- a/arch/csky/kernel/probes/uprobes.c
+++ b/arch/csky/kernel/probes/uprobes.c
@@ -3,6 +3,7 @@
* Copyright (C) 2014-2016 Pratyush Anand
*/
#include
+#include
#include
#include
#include
@@ -123,13 +124,7 @@ unsigned long
arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
struct pt_regs *regs)
{
- unsigned long ra;
-
- ra = regs->lr;
-
- regs->lr = trampoline_vaddr;
-
- return ra;
+ return __xchg(>lr, trampoline_vaddr);
}
int arch_uprobe_exception_notify(struct notifier_block *self,
diff --git a/arch/mips/kernel/irq_txx9.c b/arch/mips/kernel/irq_txx9.c
index af3ef4c9f7de1e..b5abe24ea7cfb9 100644
--- a/arch/mips/kernel/irq_txx9.c
+++ b/arch/mips/kernel/irq_txx9.c
@@ -15,6 +15,7 @@
*/
#include
#include
+#include
#include
#include
#include
@@ -159,13 +160,9 @@ void __init txx9_irq_init(unsigned long baseaddr)
int __init txx9_irq_set_pri(int
