On Wed, Apr 10, 2013 at 01:23:25PM +0530, Aneesh Kumar K.V wrote: > David Gibson <dwg@xxxxxxxxxxx> writes: > > On Wed, Apr 10, 2013 at 11:59:29AM +0530, Aneesh Kumar K.V wrote: > >> David Gibson <dwg@xxxxxxxxxxx> writes: > >> > On Thu, Apr 04, 2013 at 11:27:44AM +0530, Aneesh Kumar K.V wrote: [snip] > >> > You should really move PTE_FRAG_NR to a header so you can actually use > >> > it here rather than hard coding 16. > >> > > >> > It took me a fair while to convince myself that there is no race here > >> > with something altering mapcount and count between the atomic_read() > >> > and the atomic_sub_return(). It could do with a comment to explain > >> > why that is safe. > >> > > >> > Re-using the mapcount field for your index also seems odd, and it took > >> > me a while to convince myself that that's safe too. Wouldn't it be > >> > simpler to store a pointer to the next sub-page in the mm_context > >> > instead? You can get from that to the struct page easily enough with a > >> > shift and pfn_to_page(). > >> > >> I found using _mapcount simpler in this case. I was looking at it not > >> as an index, but rather how may fragments are mapped/used already. > > > > Except that it's actually (#fragments - 1). Using subpage pointer > > makes the fragments calculation (very slightly) harder, but the > > calculation of the table address easier. More importantly it avoids > > adding effectively an extra variable - which is then shoehorned into a > > structure not really designed to hold it. > > Even with subpage pointer we would need mm->context.pgtable_page or > something similar. We don't add any other extra variable right ?. Let me > try what you are suggesting here and see if that make it simpler. No, because the struct page * can be easily derived from the subpage pointer. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: Digital signature