On Thu, Aug 29, 2019 at 10:38:10AM +0200, Christoph Hellwig wrote: > On Thu, Aug 22, 2019 at 03:56:14PM +0530, Bharata B Rao wrote: > > +/* > > + * Bits 60:56 in the rmap entry will be used to identify the > > + * different uses/functions of rmap. > > + */ > > +#define KVMPPC_RMAP_DEVM_PFN (0x2ULL << 56) > > How did you come up with this specific value? Different usage types of RMAP array are being defined. https://patchwork.ozlabs.org/patch/1149791/ The above value is reserved for device pfn usage. > > > + > > +static inline bool kvmppc_rmap_is_devm_pfn(unsigned long pfn) > > +{ > > + return !!(pfn & KVMPPC_RMAP_DEVM_PFN); > > +} > > No need for !! when returning a bool. Also the helper seems a little > pointless, just opencoding it would make the code more readable in my > opinion. I expect similar routines for other usages of RMAP to come up. > > > +#ifdef CONFIG_PPC_UV > > +extern int kvmppc_devm_init(void); > > +extern void kvmppc_devm_free(void); > > There is no need for extern in a function declaration. > > > +static int > > +kvmppc_devm_migrate_alloc_and_copy(struct migrate_vma *mig, > > + unsigned long *rmap, unsigned long gpa, > > + unsigned int lpid, unsigned long page_shift) > > +{ > > + struct page *spage = migrate_pfn_to_page(*mig->src); > > + unsigned long pfn = *mig->src >> MIGRATE_PFN_SHIFT; > > + struct page *dpage; > > + > > + *mig->dst = 0; > > + if (!spage || !(*mig->src & MIGRATE_PFN_MIGRATE)) > > + return 0; > > + > > + dpage = kvmppc_devm_get_page(rmap, gpa, lpid); > > + if (!dpage) > > + return -EINVAL; > > + > > + if (spage) > > + uv_page_in(lpid, pfn << page_shift, gpa, 0, page_shift); > > + > > + *mig->dst = migrate_pfn(page_to_pfn(dpage)) | MIGRATE_PFN_LOCKED; > > + return 0; > > +} > > I think you can just merge this trivial helper into the only caller. Yes I can, but felt it is nicely abstracted out to a function right now. > > > +static int > > +kvmppc_devm_fault_migrate_alloc_and_copy(struct migrate_vma *mig, > > + unsigned long page_shift) > > +{ > > + struct page *dpage, *spage; > > + struct kvmppc_devm_page_pvt *pvt; > > + unsigned long pfn; > > + int ret; > > + > > + spage = migrate_pfn_to_page(*mig->src); > > + if (!spage || !(*mig->src & MIGRATE_PFN_MIGRATE)) > > + return 0; > > + if (!is_zone_device_page(spage)) > > + return 0; > > + > > + dpage = alloc_page_vma(GFP_HIGHUSER, mig->vma, mig->start); > > + if (!dpage) > > + return -EINVAL; > > + lock_page(dpage); > > + pvt = spage->zone_device_data; > > + > > + pfn = page_to_pfn(dpage); > > + ret = uv_page_out(pvt->lpid, pfn << page_shift, pvt->gpa, 0, > > + page_shift); > > + if (ret == U_SUCCESS) > > + *mig->dst = migrate_pfn(pfn) | MIGRATE_PFN_LOCKED; > > + else { > > + unlock_page(dpage); > > + __free_page(dpage); > > + } > > + return ret; > > +} > > Here we actually have two callers, but they have a fair amount of > duplicate code in them. I think you want to move that common > code (including setting up the migrate_vma structure) into this > function and maybe also give it a more descriptive name. Sure, I will give this a try. The name is already very descriptive, will come up with an appropriate name. BTW this file and the fuction prefixes in this file started out with kvmppc_hmm, switched to kvmppc_devm when HMM routines weren't used anymore. Now with the use of only non-dev versions, planning to swtich to kvmppc_uvmem_ > > > +static void kvmppc_devm_page_free(struct page *page) > > +{ > > + unsigned long pfn = page_to_pfn(page); > > + unsigned long flags; > > + struct kvmppc_devm_page_pvt *pvt; > > + > > + spin_lock_irqsave(&kvmppc_devm_pfn_lock, flags); > > + pvt = page->zone_device_data; > > + page->zone_device_data = NULL; > > + > > + bitmap_clear(kvmppc_devm_pfn_bitmap, > > + pfn - (kvmppc_devm_pgmap.res.start >> PAGE_SHIFT), 1); > > Nit: I'd just initialize pfn to the value you want from the start. > That makes the code a little easier to read, and keeps a tiny bit more > code outside the spinlock. > > unsigned long pfn = page_to_pfn(page) - > (kvmppc_devm_pgmap.res.start >> PAGE_SHIFT); > > .. > > bitmap_clear(kvmppc_devm_pfn_bitmap, pfn, 1); Sure. > > > > + kvmppc_devm_pgmap.type = MEMORY_DEVICE_PRIVATE; > > + kvmppc_devm_pgmap.res = *res; > > + kvmppc_devm_pgmap.ops = &kvmppc_devm_ops; > > + addr = memremap_pages(&kvmppc_devm_pgmap, -1); > > This -1 should be NUMA_NO_NODE for clarity. Right. Regards, Bharata.
