[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZNujJhy9fmCFwwLK@yury-ThinkPad>
Date: Tue, 15 Aug 2023 09:09:10 -0700
From: Yury Norov <yury.norov@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Bartosz Golaszewski <brgl@...ev.pl>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Thomas Gleixner <tglx@...utronix.de>,
linux-kernel@...r.kernel.org,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
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@...aro.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
Powered by blists - more mailing lists