RE: [EXTERNAL] Re: [RFC PATCH 3/4] Hyper-V/Balloon: Call add_memory() with dm_device.ha_lock.

[Tianyu Lan]

Hi Vitaly:
	Thanks for your review.
> From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> 
> > From: Tianyu Lan <Tianyu.Lan@xxxxxxxxxxxxx>
> > @@ -771,8 +767,13 @@ static void hv_online_page(struct page *pg,
> unsigned int order)
> >  	struct hv_hotadd_state *has;
> >  	unsigned long flags;
> >  	unsigned long pfn = page_to_pfn(pg);
> > +	int unlocked;
> > +
> > +	if (dm_device.lock_thread != current) {
> 
> With lock_thread checking you're trying to protect against taking the spinlock
> twice (when this is called from add_memory()) but why not just check that
> spin_is_locked() AND we sit on the same CPU as the VMBus channel
> attached to the balloon device?

Yes, that's another approach.
> 
> > +		spin_lock_irqsave(&dm_device.ha_lock, flags);
> > +		unlocked = 1;
> > +	}
> 
> We set unlocked to '1' when we're actually locked, aren't we?

The "unlocked" means ha_lock isn't hold before calling hv_online_page().

> 
> >
> > -	spin_lock_irqsave(&dm_device.ha_lock, flags);
> >  	list_for_each_entry(has, &dm_device.ha_region_list, list) {
> >  		/* The page belongs to a different HAS. */
> >  		if ((pfn < has->start_pfn) ||
> > @@ -782,7 +783,9 @@ static void hv_online_page(struct page *pg,
> unsigned int order)
> >  		hv_bring_pgs_online(has, pfn, 1UL << order);
> >  		break;
> >  	}
> > -	spin_unlock_irqrestore(&dm_device.ha_lock, flags);
> > +
> > +	if (unlocked)
> > +		spin_unlock_irqrestore(&dm_device.ha_lock, flags);
> >  }
> >
> >  static int pfn_covered(unsigned long start_pfn, unsigned long
> > pfn_cnt) @@ -860,6 +863,7 @@ static unsigned long
> handle_pg_range(unsigned long pg_start,
> >  		pg_start);
> >
> >  	spin_lock_irqsave(&dm_device.ha_lock, flags);
> > +	dm_device.lock_thread = current;
> >  	list_for_each_entry(has, &dm_device.ha_region_list, list) {
> >  		/*
> >  		 * If the pfn range we are dealing with is not in the current
> @@
> > -912,9 +916,7 @@ static unsigned long handle_pg_range(unsigned long
> pg_start,
> >  			} else {
> >  				pfn_cnt = size;
> >  			}
> > -			spin_unlock_irqrestore(&dm_device.ha_lock, flags);
> >  			hv_mem_hot_add(has->ha_end_pfn, size, pfn_cnt,
> has);
> > -			spin_lock_irqsave(&dm_device.ha_lock, flags);
> 
> Apart from the deadlock you mention in the commit message, add_memory
> does lock_device_hotplug()/unlock_device_hotplug() which is a mutex. If
> I'm not mistaken you now take the mutext under a spinlock
> (&dm_device.ha_lock). Not good.

Yes, you are right. I missed this. I will try other way. Nice catch. Thanks.

> 
> 
> >  		}
> >  		/*
> >  		 * If we managed to online any pages that were given to us,
> @@
> > -923,6 +925,7 @@ static unsigned long handle_pg_range(unsigned long
> pg_start,
> >  		res = has->covered_end_pfn - old_covered_state;
> >  		break;
> >  	}
> > +	dm_device.lock_thread = NULL;
> >  	spin_unlock_irqrestore(&dm_device.ha_lock, flags);
> >
> >  	return res;
> 
> --
> Vitaly