| Message ID | 5CAC97C30200007800225DED@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | timers: limit heap size | expand |
On 09/04/2019 14:01, Jan Beulich wrote: > @@ -463,9 +463,14 @@ static void timer_softirq_action(void) > if ( unlikely(ts->list != NULL) ) > { > /* old_limit == (2^n)-1; new_limit == (2^(n+4))-1 */ > - int old_limit = heap_metadata(heap)->limit; > - int new_limit = ((old_limit + 1) << 4) - 1; > - struct timer **newheap = xmalloc_array(struct timer *, new_limit + 1); > + unsigned int old_limit = heap_metadata(heap)->limit; > + unsigned int new_limit = ((old_limit + 1) << 4) - 1; > + struct timer **newheap = NULL; > + > + /* Don't grow the heap beyond what is representable in its metadata. */ > + if ( new_limit == (typeof(heap_metadata(heap)->limit))new_limit && > + new_limit + 1 ) > + newheap = xmalloc_array(struct timer *, new_limit + 1); It would probably be helpful to have a warn_once/print_once in the case that we do hit the metadata limit What is the new_limit + 1 for? Is it an overflow check? Given a) that we're not moving from uint16_t while we have 32bit hypervisor builds in use, and b) the calculation of new_limit will truncate before getting into a position where this overflow check would trip, I don't think it is helpful to retain. ~Andrew
On Tue, Apr 09, 2019 at 07:01:55AM -0600, Jan Beulich wrote: > @@ -544,7 +549,7 @@ static void dump_timerq(unsigned char ke > struct timers *ts; > unsigned long flags; > s_time_t now = NOW(); > - int i, j; > + unsigned int i, j; A further possible improvement is to move j within the scope of for_each_online_cpu {}. Wei.
>>> On 09.04.19 at 16:18, <wei.liu2@citrix.com> wrote: > On Tue, Apr 09, 2019 at 07:01:55AM -0600, Jan Beulich wrote: >> @@ -544,7 +549,7 @@ static void dump_timerq(unsigned char ke >> struct timers *ts; >> unsigned long flags; >> s_time_t now = NOW(); >> - int i, j; >> + unsigned int i, j; > > A further possible improvement is to move j within the scope of > for_each_online_cpu {}. In fact I did consider this, but decided against: It's "just" a debugging function, and j is not the only variable that would better move into the more narrow scope. Moving them all is beyond the scope of this patch, though. Jan
>>> On 09.04.19 at 15:38, <andrew.cooper3@citrix.com> wrote: > On 09/04/2019 14:01, Jan Beulich wrote: >> @@ -463,9 +463,14 @@ static void timer_softirq_action(void) >> if ( unlikely(ts->list != NULL) ) >> { >> /* old_limit == (2^n)-1; new_limit == (2^(n+4))-1 */ >> - int old_limit = heap_metadata(heap)->limit; >> - int new_limit = ((old_limit + 1) << 4) - 1; >> - struct timer **newheap = xmalloc_array(struct timer *, new_limit + > 1); >> + unsigned int old_limit = heap_metadata(heap)->limit; >> + unsigned int new_limit = ((old_limit + 1) << 4) - 1; >> + struct timer **newheap = NULL; >> + >> + /* Don't grow the heap beyond what is representable in its metadata. */ >> + if ( new_limit == (typeof(heap_metadata(heap)->limit))new_limit && >> + new_limit + 1 ) >> + newheap = xmalloc_array(struct timer *, new_limit + 1); > > It would probably be helpful to have a warn_once/print_once in the case > that we do hit the metadata limit I can do this, albeit the lack of the constructs you suggest will make this a little ugly. > What is the new_limit + 1 for? Is it an overflow check? Kind of: It avoids the second argument to xmalloc_array() to degenerate. > Given a) that we're not moving from uint16_t while we have 32bit > hypervisor builds in use, and b) the calculation of new_limit will > truncate before getting into a position where this overflow check would > trip, I don't think it is helpful to retain. But that's what I'm (trying to) state(ing) in the description: Making the size half a pointer's width is surely an option, which would make it 32 bits on 64-bit builds (and result in better code to be generated on x86). From a size/limit field width's perspective the changes done here would be sufficient. The left shifting in down_heap() would still need taking care of if we really were to go that route. Jan
On 09/04/2019 16:17, Jan Beulich wrote: >>>> On 09.04.19 at 15:38, <andrew.cooper3@citrix.com> wrote: >> On 09/04/2019 14:01, Jan Beulich wrote: >>> @@ -463,9 +463,14 @@ static void timer_softirq_action(void) >>> if ( unlikely(ts->list != NULL) ) >>> { >>> /* old_limit == (2^n)-1; new_limit == (2^(n+4))-1 */ >>> - int old_limit = heap_metadata(heap)->limit; >>> - int new_limit = ((old_limit + 1) << 4) - 1; >>> - struct timer **newheap = xmalloc_array(struct timer *, new_limit + >> 1); >>> + unsigned int old_limit = heap_metadata(heap)->limit; >>> + unsigned int new_limit = ((old_limit + 1) << 4) - 1; >>> + struct timer **newheap = NULL; >>> + >>> + /* Don't grow the heap beyond what is representable in its metadata. */ >>> + if ( new_limit == (typeof(heap_metadata(heap)->limit))new_limit && >>> + new_limit + 1 ) >>> + newheap = xmalloc_array(struct timer *, new_limit + 1); >> >> It would probably be helpful to have a warn_once/print_once in the case >> that we do hit the metadata limit > > I can do this, albeit the lack of the constructs you suggest will > make this a little ugly. Macros implementing such behavior would be more than welcomed. We tend to open-code WARN_ONCE/PRINT_ONCE logic in every place which is not very nice. Cheers,
--- a/xen/common/timer.c +++ b/xen/common/timer.c @@ -63,9 +63,9 @@ static struct heap_metadata *heap_metada } /* Sink down element @pos of @heap. */ -static void down_heap(struct timer **heap, int pos) +static void down_heap(struct timer **heap, unsigned int pos) { - int sz = heap_metadata(heap)->size, nxt; + unsigned int sz = heap_metadata(heap)->size, nxt; struct timer *t = heap[pos]; while ( (nxt = (pos << 1)) <= sz ) @@ -84,7 +84,7 @@ static void down_heap(struct timer **hea } /* Float element @pos up @heap. */ -static void up_heap(struct timer **heap, int pos) +static void up_heap(struct timer **heap, unsigned int pos) { struct timer *t = heap[pos]; @@ -103,8 +103,8 @@ static void up_heap(struct timer **heap, /* Delete @t from @heap. Return TRUE if new top of heap. */ static int remove_from_heap(struct timer **heap, struct timer *t) { - int sz = heap_metadata(heap)->size; - int pos = t->heap_offset; + unsigned int sz = heap_metadata(heap)->size; + unsigned int pos = t->heap_offset; if ( unlikely(pos == sz) ) { @@ -130,7 +130,7 @@ static int remove_from_heap(struct timer /* Add new entry @t to @heap. Return TRUE if new top of heap. */ static int add_to_heap(struct timer **heap, struct timer *t) { - int sz = heap_metadata(heap)->size; + unsigned int sz = heap_metadata(heap)->size; /* Fail if the heap is full. */ if ( unlikely(sz == heap_metadata(heap)->limit) ) @@ -463,9 +463,14 @@ static void timer_softirq_action(void) if ( unlikely(ts->list != NULL) ) { /* old_limit == (2^n)-1; new_limit == (2^(n+4))-1 */ - int old_limit = heap_metadata(heap)->limit; - int new_limit = ((old_limit + 1) << 4) - 1; - struct timer **newheap = xmalloc_array(struct timer *, new_limit + 1); + unsigned int old_limit = heap_metadata(heap)->limit; + unsigned int new_limit = ((old_limit + 1) << 4) - 1; + struct timer **newheap = NULL; + + /* Don't grow the heap beyond what is representable in its metadata. */ + if ( new_limit == (typeof(heap_metadata(heap)->limit))new_limit && + new_limit + 1 ) + newheap = xmalloc_array(struct timer *, new_limit + 1); if ( newheap != NULL ) { spin_lock_irq(&ts->lock); @@ -544,7 +549,7 @@ static void dump_timerq(unsigned char ke struct timers *ts; unsigned long flags; s_time_t now = NOW(); - int i, j; + unsigned int i, j; printk("Dumping timer queues:\n"); @@ -556,7 +561,7 @@ static void dump_timerq(unsigned char ke spin_lock_irqsave(&ts->lock, flags); for ( j = 1; j <= heap_metadata(ts->heap)->size; j++ ) dump_timer(ts->heap[j], now); - for ( t = ts->list, j = 0; t != NULL; t = t->list_next, j++ ) + for ( t = ts->list; t != NULL; t = t->list_next ) dump_timer(t, now); spin_unlock_irqrestore(&ts->lock, flags); }
First and foremost make timer_softirq_action() avoid growing the heap if its new size can't be stored without truncation. 64k entries is a lot, and I don't think we're at high risk of running into the issue, but I think it's better to not allow for hard to debug problems to occur in the first place. Furthermore also adjust the code such the size/limit fields becoming unsigned int would at least work from a mere sizing point of view. For this also switch various uses of plain int to unsigned int. Signed-off-by: Jan Beulich <jbeulich@suse.com>