Re: [PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain

[Bartosz Golaszewski]

On Tue, Aug 15, 2023 at 6:09 PM Yury Norov <yury.norov@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > >
> > > 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:

Did I sound smug or what? I didn't mean to. I was merely pointing to
the fact that linux is not the first to use C autopointers. They've
been around for years.

>  - 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;
>   }

Sounds good.

>
> 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:
>

This field is currently unused. I'm not sure how it ended up there,
maybe a leftover from some earlier iterations of the irq_sim. This
patch just makes use of it in the end. It may be that it should use
unsigned int. Before I change it, I'd like to hear Thomas' comments on
these changes in general.

Bart

>   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