"Alex Ng (LIS)" <ale...@microsoft.com> writes:
>> @@ -676,35 +686,63 @@ static void hv_mem_hot_add(unsigned long start,
>> unsigned long size,
>>
>> static void hv_online_page(struct page *pg) {
>> - struct list_head *cur;
>> struct hv_hotadd_state *has;
>> + struct hv_hotadd_gap *gap;
>> unsigned long cur_start_pgp;
>> unsigned long cur_end_pgp;
>> + bool is_gap = false;
>>
>> list_for_each(cur, &dm_device.ha_region_list) {
>> has = list_entry(cur, struct hv_hotadd_state, list);
>> cur_start_pgp = (unsigned long)
>> + pfn_to_page(has->start_pfn);
>> + cur_end_pgp = (unsigned long)pfn_to_page(has->end_pfn);
>> +
>> + /* The page belongs to a different HAS. */
>> + if (((unsigned long)pg < cur_start_pgp) ||
>> + ((unsigned long)pg >= cur_end_pgp))
>> + continue;
>> +
>> + cur_start_pgp = (unsigned long)
>> pfn_to_page(has->covered_start_pfn);
>> cur_end_pgp = (unsigned long)pfn_to_page(has-
>> >covered_end_pfn);
>>
>> - if (((unsigned long)pg >= cur_start_pgp) &&
>> - ((unsigned long)pg < cur_end_pgp)) {
>> - /*
>> - * This frame is currently backed; online the
>> - * page.
>> - */
>> - __online_page_set_limits(pg);
>> - __online_page_increment_counters(pg);
>> - __online_page_free(pg);
>> + /* The page is not backed. */
>> + if (((unsigned long)pg < cur_start_pgp) ||
>> + ((unsigned long)pg >= cur_end_pgp))
>> + continue;
>> +
>> + /* Check for gaps. */
>> + list_for_each_entry(gap, &has->gap_list, list) {
>> + cur_start_pgp = (unsigned long)
>> + pfn_to_page(gap->start_pfn);
>> + cur_end_pgp = (unsigned long)
>> + pfn_to_page(gap->end_pfn);
>> + if (((unsigned long)pg >= cur_start_pgp) &&
>> + ((unsigned long)pg < cur_end_pgp)) {
>> + is_gap = true;
>> + break;
>> + }
>> }
>> +
>> + if (is_gap)
>> + break;
>> +
>> + /* This frame is currently backed; online the page. */
>> + __online_page_set_limits(pg);
>> + __online_page_increment_counters(pg);
>> + __online_page_free(pg);
>> + break;
>> }
>> }
>>
>
> We may need to add similar logic to check for gaps in hv_bring_pgs_online().
>
> [...]
Yes, probably, I'll take a look and try to refactor the onlinig code in
a separate function to avoid duplication.
>> static unsigned long handle_pg_range(unsigned long pg_start, @@ -834,13
>> +881,19 @@ static unsigned long process_hot_add(unsigned long pg_start,
>> unsigned long rg_size)
>> {
>> struct hv_hotadd_state *ha_region = NULL;
>> + int covered;
>>
>> if (pfn_cnt == 0)
>> return 0;
>>
>> - if (!dm_device.host_specified_ha_region)
>> - if (pfn_covered(pg_start, pfn_cnt))
>> + if (!dm_device.host_specified_ha_region) {
>> + covered = pfn_covered(pg_start, pfn_cnt);
>> + if (covered < 0)
>> + return 0;
>
> If the hot-add pages aren't covered by any region, then shouldn't it fall
> through instead of returning?
> That way the new ha_region can be added to the list and we hot-add the
> pages accordingly.
I was under an impression this is impossible:
hot_add_req()/process_hot_add() will create a new region in this
case. 'covered < 0' was added to handle one particular error: failure to
allocate memory to record gap (struct hv_hotadd_gap) and I don't have a
better idea how to handle this: if we can't remember the gap we'll crash
later on onlining...
--
Vitaly
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
