Messages in this thread | | | Date | Tue, 15 Aug 2023 09:09:10 -0700 | From | Yury Norov <> | Subject | Re: [PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain |
| |
On Tue, Aug 15, 2023 at 01:38:49PM +0300, Andy Shevchenko wrote: > On Sat, Aug 12, 2023 at 09:44:54PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > If the device providing simulated interrupts is unbound (real life > > example: gpio-sim is disabled with users that didn't free their irqs) > > and removes the simulated domain while interrupts are still requested, > > we will hit memory issues when they are eventually freed and the > > mappings destroyed in the process. > > > > Specifically we'll access freed memory in __irq_domain_deactivate_irq(). > > > > Dispose of all mappings before removing the simulator domain. > > ... > > > +#include <linux/list.h> > > Maybe ordered? > > > #include <linux/irq.h> > > #include <linux/irq_sim.h> > > #include <linux/irq_work.h> > > ... > > > @@ -16,12 +17,14 @@ struct irq_sim_work_ctx { > > unsigned int irq_count; > > unsigned long *pending; > > struct irq_domain *domain; > > + struct list_head irqs; > > }; > > > > struct irq_sim_irq_ctx { > > int irqnum; > > bool enabled; > > struct irq_sim_work_ctx *work_ctx; > > > + struct list_head siblings; > > You can reduce the code size by moving this to be the first member. > Not sure about struct irq_sim_work_ctx, you can play with bloat-o-meter.
Pahole you meant?
yury:linux$ pahole -C irq_sim_irq_ctx /sys/kernel/btf/vmlinux struct irq_sim_irq_ctx { int irqnum; /* 0 4 */ bool enabled; /* 4 1 */ /* XXX 3 bytes hole, try to pack */ struct irq_sim_work_ctx * work_ctx; /* 8 8 */ /* size: 16, cachelines: 1, members: 3 */ /* sum members: 13, holes: 1, sum holes: 3 */ /* last cacheline: 16 bytes */ };
In this particular case, there will be no hole because list head position (16) will be aligned to sizeof(struct list_head) == 16.
But as Bartosz said in the other email, "it's just good practice resulting from years of" kernel coding to have: - members declared strongly according to the logic of the code, and if no strong preference: - list head be the first element of the structure, to let compiler avoid generating offsets when traversing lists; - put elements of greater size at the beginning, so no holes will be emitted like in the example above.
So I'd suggest:
struct irq_sim_irq_ctx { struct list_head siblings; struct irq_sim_work_ctx *work_ctx; int irqnum; bool enabled; } Again, if there's NO ANY reason to have the irq number at the beginning.
While here, I wonder, why irqnum is signed? Looking at the very first random function in kernel/irq/irq_sim.c, I see that it's initialized from a function returning unsigned value:
static void irq_sim_handle_irq(struct irq_work *work) { struct irq_sim_work_ctx *work_ctx; unsigned int offset = 0; int irqnum; work_ctx = container_of(work, struct irq_sim_work_ctx, work); while (!bitmap_empty(work_ctx->pending, work_ctx->irq_count)) { offset = find_next_bit(work_ctx->pending, work_ctx->irq_count, offset); clear_bit(offset, work_ctx->pending); irqnum = irq_find_mapping(work_ctx->domain, offset); handle_simple_irq(irq_to_desc(irqnum)); } }
Thanks, Yury
| |