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>
> +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()
> + 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.
-Andi
> + if (!new_entries)
> + return -ENOMEM;
> + memcpy(new_entries, entries,
> + erst_record_id_cache.len * sizeof(entries[0]));
> + if (erst_record_id_cache.size < PAGE_SIZE)
> + kfree(entries);
> + else
> + vfree(entries);
-Andi
--
ak@xxxxxxxxxxxxxxx -- Speaking for myself only.
--
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
