Changeset View
Standalone View
sys/kern/subr_intr.c
Show First 20 Lines • Show All 395 Lines • ▼ Show 20 Lines | |||||
* | * | ||||
* 1. Handles are always allocated forward, so handles are not recycled | * 1. Handles are always allocated forward, so handles are not recycled | ||||
* immediately. However, if only one free handle left which is reused | * immediately. However, if only one free handle left which is reused | ||||
* constantly... | * constantly... | ||||
*/ | */ | ||||
static inline int | static inline int | ||||
isrc_alloc_irq(struct intr_irqsrc *isrc) | isrc_alloc_irq(struct intr_irqsrc *isrc) | ||||
{ | { | ||||
u_int maxirqs, irq; | u_int maxirqs; | ||||
mtx_assert(&isrc_table_lock, MA_OWNED); | mtx_assert(&isrc_table_lock, MA_OWNED); | ||||
maxirqs = intr_nirq; | maxirqs = intr_nirq; | ||||
markj: I think there is no point in having `maxirqs`, just reference `intr_nirq` directly. | |||||
ehem_freebsd_m5p.comAuthorUnsubmitted Done Inline ActionsThere is a subtle gotcha here, pointed to by D29327. maxirqs is unsigned intr_nirq is signed. While it is unlikely for intr_nirq to be large enough for this to matter it could. In fact INTR_IRQ_INVALID is -1 with intr_nirq being signed. Having typed that, what is presently in my development tree nukes maxirqs. I've been waiting for comments before finishing cleanup and updating D29310. ehem_freebsd_m5p.com: There is a subtle gotcha here, pointed to by D29327. `maxirqs` is //unsigned// `intr_nirq` is… | |||||
if (irq_next_free >= maxirqs) | |||||
return (ENOSPC); | |||||
for (irq = irq_next_free; irq < maxirqs; irq++) { | while (irq_next_free < maxirqs) { | ||||
if (irq_sources[irq] == NULL) | if (irq_sources[irq_next_free] == NULL) | ||||
goto found; | goto found; | ||||
++irq_next_free; | |||||
} | } | ||||
for (irq = 0; irq < irq_next_free; irq++) { | |||||
if (irq_sources[irq] == NULL) | |||||
goto found; | |||||
} | |||||
irq_next_free = maxirqs; | |||||
return (ENOSPC); | return (ENOSPC); | ||||
found: | found: | ||||
Done Inline Actionslog () is not the standard way to log kernel problems. We use printf() or device_printf() for this. In addition, in this case, I think that the requester is responsible for reporting the problem. mmel: log () is not the standard way to log kernel problems. We use printf() or device_printf() for… | |||||
isrc->isrc_irq = irq; | isrc->isrc_irq = irq_next_free; | ||||
irq_sources[irq] = isrc; | irq_sources[irq_next_free] = isrc; | ||||
irq_next_free = irq + 1; | ++irq_next_free; | ||||
if (irq_next_free >= maxirqs) | |||||
irq_next_free = 0; | |||||
return (0); | return (0); | ||||
} | } | ||||
/* | /* | ||||
* Free unique interrupt number (resource handle) from interrupt source. | * Free unique interrupt number (resource handle) from interrupt source. | ||||
*/ | */ | ||||
static inline int | static inline int | ||||
isrc_free_irq(struct intr_irqsrc *isrc) | isrc_free_irq(struct intr_irqsrc *isrc) | ||||
{ | { | ||||
mtx_assert(&isrc_table_lock, MA_OWNED); | mtx_assert(&isrc_table_lock, MA_OWNED); | ||||
if (isrc->isrc_irq >= intr_nirq) | if (isrc->isrc_irq >= intr_nirq) | ||||
return (EINVAL); | return (EINVAL); | ||||
if (irq_sources[isrc->isrc_irq] != isrc) | if (irq_sources[isrc->isrc_irq] != isrc) | ||||
return (EINVAL); | return (EINVAL); | ||||
/* Signal availability to isrc_alloc_irq(). */ | |||||
Done Inline ActionsExisting comments in the file are, for the most part, capitalized and end with a period. mhorne: Existing comments in the file are, for the most part, capitalized and end with a period. | |||||
if (isrc->isrc_irq < irq_next_free) | |||||
irq_next_free = isrc->isrc_irq; | |||||
Done Inline ActionsThis is wrong. the irq_next_free is used as start index of full table lookup -> see second cycle in isrc_alloc_irq() and (mainly) a comment above this function . This patch violates assumption in mentioned comment. But I don't see where exactly is problem. mmel: This is wrong. the irq_next_free is used as start index of full table lookup -> see second… | |||||
Done Inline ActionsYou're missing what occurs in the table of interrupts fills. If isrc_alloc_irq() finds the table is presently full (it falls through the second loop), then it will set:
This in turn causes isrc_alloc_irq() to exit early at line 409. This means the table will never be rescanned and isrc_alloc_irq() will return ENOSPC no matter how many interrupts are released. If irq_next_free >= intr_nirq, then the table is presently known to be full and scanning is currently disabled. As such setting irq_next_free to something less than intr_nirq is required to restart scanning. The value isrc->isrc_irq is appropriate since it is a now known to be available and the first entry the table scan looks at will be available. I can even argue for the approach of unconditionally setting irq_next_free = isrc->isrc_irq, since it may make isrc_alloc_irq() faster by guaranteeing irq_next_free is presently pointing to an available entry. This avoids a branch and read from memory. On the flip side this potentially makes allocation order a bit interesting. ehem_freebsd_m5p.com: You're missing what occurs in the table of interrupts fills. If isrc_alloc_irq() finds the… | |||||
Not Done Inline Actions
Ahh, yes, sure. I overlooked the 'if' condition on line 409, sorry for this.
I strongly disagree with that. The original goal of the proposal is to detach the allocation order from the release order. For me, it is part of the (implicit) security of standard code, although we are talking more or less about a hypothetical situation. Also this code is not on hot-path. mmel: > You're missing what occurs in the table of interrupts fills. If isrc_alloc_irq() finds the… | |||||
Done Inline Actions
I'll concede this shouldn't be hot-path, but that is partially due to the original authors paying attention to avoid performance issues. The very existence of irq_next_free points to them having paid attention. Instead of always scanning from the start, it scans from where it last left off. Instead of scanning the full table before determining it is full, it records that it is full. ehem_freebsd_m5p.com: > > I can even argue for the approach of unconditionally setting irq_next_free = isrc->isrc_irq… | |||||
Not Done Inline ActionsThe performance of isrc_alloc_irq() after the table is full is not important, and arguably not important at all. It is better to do the simpler thing, which is to decouple isrc_alloc_irq() and isrc_free_irq() by having isrc_alloc_irq() do a full scan of the table after the cursor has wrapped around. In other words, I think isrc_alloc_irq() should simply set irq_next_free to 0 before returning ENOSPC, and isrc_free_irq() should not change at all. markj: The performance of isrc_alloc_irq() after the table is full is not important, and arguably not… | |||||
irq_sources[isrc->isrc_irq] = NULL; | irq_sources[isrc->isrc_irq] = NULL; | ||||
isrc->isrc_irq = INTR_IRQ_INVALID; /* just to be safe */ | isrc->isrc_irq = INTR_IRQ_INVALID; /* just to be safe */ | ||||
Not Done Inline ActionsWhat about this? /* * If we are recovering from a full isrc table state, next allocation * should perform a full table scan. This ensures that order of * allocations is separated from the release sequences. */ if (irq_next_free >= intr_nirq) irq_next_free = 0; mmel: What about this?
```
/*
* If we are recovering from a full isrc table state, next… | |||||
Done Inline ActionsIf the table was full, then our newly freed entry will be the only available one. The next call to isrc_alloc_irq() will find it and terminate quickly. Setting to zero means the next isrc_alloc_irq() call will scan more than half the table, since the low-numbered entries are occupied by permanently allocated entries and then it will on average be halfway along (setting to intr_nirq/2 would likely offer measurably better performance). ehem_freebsd_m5p.com: If the table //was// full, then our newly freed entry will be the only available one. The next… | |||||
return (0); | return (0); | ||||
} | } | ||||
/* | /* | ||||
* Initialize interrupt source and register it into global interrupt table. | * Initialize interrupt source and register it into global interrupt table. | ||||
*/ | */ | ||||
int | int | ||||
intr_isrc_register(struct intr_irqsrc *isrc, device_t dev, u_int flags, | intr_isrc_register(struct intr_irqsrc *isrc, device_t dev, u_int flags, | ||||
▲ Show 20 Lines • Show All 1,277 Lines • Show Last 20 Lines |
I think there is no point in having maxirqs, just reference intr_nirq directly.