Hi, Andi,
On Fri, 2010-10-22 at 20:04 +0800, Andi Kleen wrote:
> On Wed, Oct 20, 2010 at 09:36:52AM +0800, Huang Ying wrote:
> > 1
> > 2
> > 3
> > 4
> > -1
> > -1
> >
> > where -1 signals there is no more record ID.
> >
> > Reader 1 has no chance to check record 2 and 4, while reader 2 has no
> > chance to check record 1 and 3. And any other GET_NEXT_RECORD_ID will
> > return -1, that is, other readers will has no chance to check any
> > record even they are not cleared by anyone.
> >
> > This makes raw GET_NEXT_RECORD_ID not suitable for usage of multiple
> > users.
> >
> > To solve the issue, an in memory ERST record ID cache is designed and
> > implemented. When enumerating record ID, the ID returned by
> > GET_NEXT_RECORD_ID is added into cache in addition to be returned to
> > caller. So other readers can check the cache to get all record ID
> > available.
>
> Generally it looks ok, just a minor cleanup nit below.
>
> Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
Thanks.
> > +static int erst_record_id_cache_add_one(void)
> > +{
> > + u64 id, prev_id, first_id;
> > + int i, rc;
> > + struct erst_record_id_entry *entries;
> > + unsigned long flags;
> > +
> > + id = prev_id = first_id = APEI_ERST_INVALID_RECORD_ID;
> > +retry:
> > + spin_lock_irqsave(&erst_lock, flags);
> > + rc = __erst_get_next_record_id(&id);
> > + spin_unlock_irqrestore(&erst_lock, flags);
> > + if (rc == -ENOENT)
> > + return 0;
> > + if (rc)
> > + return rc;
> > + if (id == APEI_ERST_INVALID_RECORD_ID)
> > + return 0;
> > + /* can not skip current ID, or look back to first ID */
> > + if (id == prev_id || id == first_id)
> > + return 0;
> > + if (first_id == APEI_ERST_INVALID_RECORD_ID)
> > + first_id = id;
> > + prev_id = id;
> > +
> > + entries = erst_record_id_cache.entries;
> > + for (i = 0; i < erst_record_id_cache.len; i++) {
> > + if (!entries[i].cleared && entries[i].id == id)
> > + break;
> > + }
> > + /* record id already in cache, try next */
> > + if (i < erst_record_id_cache.len)
> > + goto retry;
> > + if (erst_record_id_cache.len >= erst_record_id_cache.size) {
> > + int new_size, alloc_size;
> > + struct erst_record_id_entry *new_entries;
> > +
> > + new_size = erst_record_id_cache.size * 2;
> > + new_size = max_t(int, new_size, ERST_RECORD_ID_CACHE_SIZE_MIN);
> > + new_size = min_t(int, new_size, ERST_RECORD_ID_CACHE_SIZE_MAX);
>
> This is clamp_t()
Yes. Will change it.
> > + if (new_size <= erst_record_id_cache.size) {
> > + if (printk_ratelimit())
> > + pr_warning(FW_WARN ERST_PFX
> > + "too many record ID!\n");
> > + return 0;
> > + }
> > + alloc_size = new_size * sizeof(struct erst_record_id_entry);
> > + if (alloc_size < PAGE_SIZE)
> > + new_entries = kmalloc(alloc_size, GFP_KERNEL);
> > + else
> > + new_entries = vmalloc(alloc_size);
>
> This is essentially kremalloc with vmalloc. Since this a common
> pattern it would be nicer to put a generic helper for this somewhere.
Yes. But will try to do that in another patch.
Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
