On 11/21/19 12:03 AM, Christoph Hellwig wrote:
On Wed, Nov 20, 2019 at 11:13:32PM -0800, John Hubbard wrote:
There are four locations in gup.c that have a fair amount of code duplication. This means that changing one requires making the same changes in four places, not to mention reading the same code four times, and wondering if there are subtle differences.
Factor out the common code into static functions, thus reducing the overall line count and the code's complexity.
Also, take the opportunity to slightly improve the efficiency of the error cases, by doing a mass subtraction of the refcount, surrounded by get_page()/put_page().
Also, further simplify (slightly), by waiting until the the successful end of each routine, to increment *nr.
Any reason for the spurious underscore in the function name?
argghh, I just fixed that, but applied the fix to the wrong patch! So now patch 17 ("mm/gup: track FOLL_PIN pages") is improperly renaming it, instead of this patch naming it correctly in the first place. Will fix.
Otherwise this looks fine and might be a worthwhile cleanup to feed Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig hch@lst.de
Thanks for the reviews! Say, it sounds like your view here is that this series should be targeted at 5.6 (not 5.5), is that what you have in mind? And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
thanks,
On Thu 21-11-19 00:29:59, John Hubbard wrote:
On 11/21/19 12:03 AM, Christoph Hellwig wrote:
Otherwise this looks fine and might be a worthwhile cleanup to feed Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig hch@lst.de
Thanks for the reviews! Say, it sounds like your view here is that this series should be targeted at 5.6 (not 5.5), is that what you have in mind? And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
Yeah, actually I feel the same. The merge window is going to open on Sunday and the series isn't still fully baked and happily sitting in linux-next (and larger changes should really sit in linux-next for at least a week, preferably two, before the merge window opens to get some reasonable test coverage). So I'd take out the independent easy patches that are already reviewed, get them merged into Andrew's (or whatever other appropriate tree) now so that they get at least a week of testing in linux-next before going upstream. And the more involved bits will have to wait for 5.6 - which means let's just continue working on them as we do now because ideally in 4 weeks we should have them ready with all the reviews so that they can be picked up and integrated into linux-next.
Honza
On 11/21/19 1:49 AM, Jan Kara wrote:
On Thu 21-11-19 00:29:59, John Hubbard wrote:
On 11/21/19 12:03 AM, Christoph Hellwig wrote:
Otherwise this looks fine and might be a worthwhile cleanup to feed Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig hch@lst.de
Thanks for the reviews! Say, it sounds like your view here is that this series should be targeted at 5.6 (not 5.5), is that what you have in mind? And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
Yeah, actually I feel the same. The merge window is going to open on Sunday and the series isn't still fully baked and happily sitting in linux-next (and larger changes should really sit in linux-next for at least a week, preferably two, before the merge window opens to get some reasonable test coverage). So I'd take out the independent easy patches that are already reviewed, get them merged into Andrew's (or whatever other appropriate tree) now so that they get at least a week of testing in linux-next before going upstream. And the more involved bits will have to wait for 5.6 - which means let's just continue working on them as we do now because ideally in 4 weeks we should have them ready with all the reviews so that they can be picked up and integrated into linux-next.
Honza
OK, thanks for spelling it out. I'll shift over to getting the easy patches prepared for 5.5, for now.
thanks,
On Thu 21-11-19 00:29:59, John Hubbard wrote:
Otherwise this looks fine and might be a worthwhile cleanup to feed Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig hch@lst.de
Thanks for the reviews! Say, it sounds like your view here is that this series should be targeted at 5.6 (not 5.5), is that what you have in mind? And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
One more note :) If you are going to push pin_user_pages() interfaces (which I'm fine with), it would probably make sense to push also the put_user_pages() -> unpin_user_pages() renaming so that that inconsistency in naming does not exist in the released upstream kernel.
Honza
On 11/21/19 1:54 AM, Jan Kara wrote:
On Thu 21-11-19 00:29:59, John Hubbard wrote:
Otherwise this looks fine and might be a worthwhile cleanup to feed Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig hch@lst.de
Thanks for the reviews! Say, it sounds like your view here is that this series should be targeted at 5.6 (not 5.5), is that what you have in mind? And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
One more note :) If you are going to push pin_user_pages() interfaces (which I'm fine with), it would probably make sense to push also the put_user_pages() -> unpin_user_pages() renaming so that that inconsistency in naming does not exist in the released upstream kernel.
Honza
Yes, that's what this patch series does. But I'm not sure if "push" here means, "push out: defer to 5.6", "push (now) into 5.5", or "advocate for"?
I will note that it's not going to be easy to rename in one step, now that this is being split up. Because various put_user_pages()-based items are going into 5.5 via different maintainer trees now. Probably I'd need to introduce unpin_user_page() alongside put_user_page()...thoughts?
thanks,
On Thu 21-11-19 18:54:02, John Hubbard wrote:
On 11/21/19 1:54 AM, Jan Kara wrote:
On Thu 21-11-19 00:29:59, John Hubbard wrote:
Otherwise this looks fine and might be a worthwhile cleanup to feed Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig hch@lst.de
Thanks for the reviews! Say, it sounds like your view here is that this series should be targeted at 5.6 (not 5.5), is that what you have in mind? And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
One more note :) If you are going to push pin_user_pages() interfaces (which I'm fine with), it would probably make sense to push also the put_user_pages() -> unpin_user_pages() renaming so that that inconsistency in naming does not exist in the released upstream kernel.
HonzaYes, that's what this patch series does. But I'm not sure if "push" here means, "push out: defer to 5.6", "push (now) into 5.5", or "advocate for"?
I meant to include the patch in the "for 5.5" batch.
I will note that it's not going to be easy to rename in one step, now that this is being split up. Because various put_user_pages()-based items are going into 5.5 via different maintainer trees now. Probably I'd need to introduce unpin_user_page() alongside put_user_page()...thoughts?
Yes, I understand that moving that patch from the end of the series would cause fair amount of conflicts. I was hoping that you could generate the patch with sed/Coccinelle and then rebasing what remains for 5.6 on top of that patch should not be that painful so overall it should not be that much work. But I may be wrong so if it proves to be too tedious, let's just postpone the renaming to 5.6. I don't find having both unpin_user_page() and put_user_page() a better alternative to current state. Thanks!
Honza
dri-devel@lists.freedesktop.org
-
Jan Kara -
John Hubbard