| Message ID | 20220419234328.10346-1-eng.alaamohamedsoliman.am@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f31076a6b2391896088fa81755a892213f07788e |
| Headers | show |
| Series | [v2] xen: Convert kmap() to kmap_local_page() | expand |
On Wed, 20 Apr 2022, Alaa Mohamed wrote: > kmap() is being deprecated and these usages are all local to the thread > so there is no reason kmap_local_page() can't be used. > > Replace kmap() calls with kmap_local_page(). OK, so from a Coccinelle point of view, could we do @@ expression e1,e2,x,f; @@ e1 = - kmap + kmap_local_page (e2) ... when != x = e1 // not stored in any location and not passed to another function when != f(...,e1,...) when != x = e2 when != f(...,e2,...) -kunmap(e2) +kunmap_local(e1) julia > > Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com> > --- > changes in V2: > -edit commit subject > -edit commit message > --- > drivers/xen/gntalloc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c > index 4849f94372a4..55acb32842a3 100644 > --- a/drivers/xen/gntalloc.c > +++ b/drivers/xen/gntalloc.c > @@ -178,9 +178,9 @@ static void __del_gref(struct gntalloc_gref *gref) > unsigned long addr; > > if (gref->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) { > - uint8_t *tmp = kmap(gref->page); > + uint8_t *tmp = kmap_local_page(gref->page); > tmp[gref->notify.pgoff] = 0; > - kunmap(gref->page); > + kunmap_local(tmp); > } > if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) { > notify_remote_via_evtchn(gref->notify.event); > -- > 2.35.2 > > >
On mercoledì 20 aprile 2022 08:03:05 CEST Julia Lawall wrote: > > On Wed, 20 Apr 2022, Alaa Mohamed wrote: > > > kmap() is being deprecated and these usages are all local to the thread > > so there is no reason kmap_local_page() can't be used. > > > > Replace kmap() calls with kmap_local_page(). > > OK, so from a Coccinelle point of view, could we do > > @@ > expression e1,e2,x,f; > @@ > > e1 = > - kmap > + kmap_local_page > (e2) > ... when != x = e1 // not stored in any location and not passed to another function > when != f(...,e1,...) > when != x = e2 > when != f(...,e2,...) > -kunmap(e2) > +kunmap_local(e1) > > julia > I've never spent sufficient time to understand properly the syntax and semantics of expressions of Coccinelle. However, thanks Julia, this code looks good and can be very helpful. Only a minor objection... it doesn't tell when 'e2' has been allocated within the same function where the kmap() call is. In the particular case that I cite above, I'd prefer to remove the allocation of the page (say with alloc_page()) and convert kmap() /kunmap() to use kmalloc() / kfree(). Fox example, this is done in the following patch: commit 633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from sgx_ioc_enclave_init()") from Ira Weiny. Can Coccinelle catch also those special cases where a page that is passed to kmap() is allocated within that same function (vs. being passed as argument to this function) and, if so, propose a replacement with kmalloc()? Thanks, Fabio
On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > On mercoledì 20 aprile 2022 08:03:05 CEST Julia Lawall wrote: > > > > On Wed, 20 Apr 2022, Alaa Mohamed wrote: > > > > > kmap() is being deprecated and these usages are all local to the thread > > > so there is no reason kmap_local_page() can't be used. > > > > > > Replace kmap() calls with kmap_local_page(). > > > > OK, so from a Coccinelle point of view, could we do > > > > @@ > > expression e1,e2,x,f; > > @@ > > > > e1 = > > - kmap > > + kmap_local_page > > (e2) > > ... when != x = e1 // not stored in any location and not passed to > another function > > when != f(...,e1,...) > > when != x = e2 > > when != f(...,e2,...) > > -kunmap(e2) > > +kunmap_local(e1) > > > > julia > > > > I've never spent sufficient time to understand properly the syntax and > semantics of expressions of Coccinelle. However, thanks Julia, this code > looks good and can be very helpful. > > Only a minor objection... it doesn't tell when 'e2' has been allocated > within the same function where the kmap() call is. OK, thanks for pointing that out. That seems like a key point that should be mentioned in log messages. It's not even visible in the context lines around the patch. > > In the particular case that I cite above, I'd prefer to remove the > allocation of the page (say with alloc_page()) and convert kmap() /kunmap() > to use kmalloc() / kfree(). > > Fox example, this is done in the following patch: > > commit 633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from > sgx_ioc_enclave_init()") from Ira Weiny. > > Can Coccinelle catch also those special cases where a page that is passed > to kmap() is allocated within that same function (vs. being passed as > argument to this function) and, if so, propose a replacement with > kmalloc()? Sure. I'll take a look at the precise example Alaa started with and see if I can come up with something. thanks, julia
On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > On mercoledì 20 aprile 2022 08:03:05 CEST Julia Lawall wrote: > > > > On Wed, 20 Apr 2022, Alaa Mohamed wrote: > > > > > kmap() is being deprecated and these usages are all local to the thread > > > so there is no reason kmap_local_page() can't be used. > > > > > > Replace kmap() calls with kmap_local_page(). > > > > OK, so from a Coccinelle point of view, could we do > > > > @@ > > expression e1,e2,x,f; > > @@ > > > > e1 = > > - kmap > > + kmap_local_page > > (e2) > > ... when != x = e1 // not stored in any location and not passed to > another function > > when != f(...,e1,...) > > when != x = e2 > > when != f(...,e2,...) > > -kunmap(e2) > > +kunmap_local(e1) > > > > julia > > > > I've never spent sufficient time to understand properly the syntax and > semantics of expressions of Coccinelle. However, thanks Julia, this code > looks good and can be very helpful. > > Only a minor objection... it doesn't tell when 'e2' has been allocated > within the same function where the kmap() call is. > > In the particular case that I cite above, I'd prefer to remove the > allocation of the page (say with alloc_page()) and convert kmap() /kunmap() > to use kmalloc() / kfree(). > > Fox example, this is done in the following patch: > > commit 633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from > sgx_ioc_enclave_init()") from Ira Weiny. > > Can Coccinelle catch also those special cases where a page that is passed > to kmap() is allocated within that same function (vs. being passed as > argument to this function) and, if so, propose a replacement with > kmalloc()? It looks complex in this case, because the allocation is in another function, and it is passed to another function. julia > > Thanks, > > Fabio > > > >
On mercoledì 20 aprile 2022 15:40:10 CEST Julia Lawall wrote: > > On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > > > On mercoledì 20 aprile 2022 08:03:05 CEST Julia Lawall wrote: > > > > > > On Wed, 20 Apr 2022, Alaa Mohamed wrote: > > > > > > > kmap() is being deprecated and these usages are all local to the thread > > > > so there is no reason kmap_local_page() can't be used. > > > > > > > > Replace kmap() calls with kmap_local_page(). > > > > > > OK, so from a Coccinelle point of view, could we do > > > > > > @@ > > > expression e1,e2,x,f; > > > @@ > > > > > > e1 = > > > - kmap > > > + kmap_local_page > > > (e2) > > > ... when != x = e1 // not stored in any location and not passed to > > another function > > > when != f(...,e1,...) > > > when != x = e2 > > > when != f(...,e2,...) > > > -kunmap(e2) > > > +kunmap_local(e1) > > > > > > julia > > > > > > > I've never spent sufficient time to understand properly the syntax and > > semantics of expressions of Coccinelle. However, thanks Julia, this code > > looks good and can be very helpful. > > > > Only a minor objection... it doesn't tell when 'e2' has been allocated > > within the same function where the kmap() call is. > > > > In the particular case that I cite above, I'd prefer to remove the > > allocation of the page (say with alloc_page()) and convert kmap() / kunmap() > > to use kmalloc() / kfree(). > > > > Fox example, this is done in the following patch: > > > > commit 633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from > > sgx_ioc_enclave_init()") from Ira Weiny. > > > > Can Coccinelle catch also those special cases where a page that is passed > > to kmap() is allocated within that same function (vs. being passed as > > argument to this function) and, if so, propose a replacement with > > kmalloc()? > > It looks complex in this case, because the allocation is in another > function, and it is passed to another function. This is not the special case I was talking about. In this case your code for Coccinelle tells the right proposal and it is exactly what Alaa did in her patch (which is good!). I'm talking about other special cases like the one I pointed to with the link I provided. I'm sorry if my bad English made you think that Alaa's patch was one of those cases where the page is allocated within the same function where kmap() is. I hope that now I've been clearer :) Thanks, Fabio
On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > On mercoledì 20 aprile 2022 15:40:10 CEST Julia Lawall wrote: > > > > On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > > > > > On mercoledì 20 aprile 2022 08:03:05 CEST Julia Lawall wrote: > > > > > > > > On Wed, 20 Apr 2022, Alaa Mohamed wrote: > > > > > > > > > kmap() is being deprecated and these usages are all local to the > thread > > > > > so there is no reason kmap_local_page() can't be used. > > > > > > > > > > Replace kmap() calls with kmap_local_page(). > > > > > > > > OK, so from a Coccinelle point of view, could we do > > > > > > > > @@ > > > > expression e1,e2,x,f; > > > > @@ > > > > > > > > e1 = > > > > - kmap > > > > + kmap_local_page > > > > (e2) > > > > ... when != x = e1 // not stored in any location and not passed to > > > another function > > > > when != f(...,e1,...) > > > > when != x = e2 > > > > when != f(...,e2,...) > > > > -kunmap(e2) > > > > +kunmap_local(e1) > > > > > > > > julia > > > > > > > > > > I've never spent sufficient time to understand properly the syntax and > > > semantics of expressions of Coccinelle. However, thanks Julia, this > code > > > looks good and can be very helpful. > > > > > > Only a minor objection... it doesn't tell when 'e2' has been allocated > > > within the same function where the kmap() call is. > > > > > > In the particular case that I cite above, I'd prefer to remove the > > > allocation of the page (say with alloc_page()) and convert kmap() / > kunmap() > > > to use kmalloc() / kfree(). > > > > > > Fox example, this is done in the following patch: > > > > > > commit 633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from > > > sgx_ioc_enclave_init()") from Ira Weiny. > > > > > > Can Coccinelle catch also those special cases where a page that is > passed > > > to kmap() is allocated within that same function (vs. being passed as > > > argument to this function) and, if so, propose a replacement with > > > kmalloc()? > > > > It looks complex in this case, because the allocation is in another > > function, and it is passed to another function. > > This is not the special case I was talking about. In this case your code > for Coccinelle tells the right proposal and it is exactly what Alaa did in > her patch (which is good!). > > I'm talking about other special cases like the one I pointed to with the > link I provided. I'm sorry if my bad English made you think that Alaa's > patch was one of those cases where the page is allocated within the same > function where kmap() is. > > I hope that now I've been clearer :) Ah, sorry for the misunderstanding. If you have an example, I can take a look and propose something for this special case. julia
On mercoledì 20 aprile 2022 15:57:14 CEST Julia Lawall wrote: > > On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > > > On mercoledì 20 aprile 2022 15:40:10 CEST Julia Lawall wrote: > > > > > > On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > > > > > > > On mercoledì 20 aprile 2022 08:03:05 CEST Julia Lawall wrote: > > > > > > > > > > On Wed, 20 Apr 2022, Alaa Mohamed wrote: > > > > > > > > > > > kmap() is being deprecated and these usages are all local to the > > thread > > > > > > so there is no reason kmap_local_page() can't be used. > > > > > > > > > > > > Replace kmap() calls with kmap_local_page(). > > > > > > > > > > OK, so from a Coccinelle point of view, could we do > > > > > > > > > > @@ > > > > > expression e1,e2,x,f; > > > > > @@ > > > > > > > > > > e1 = > > > > > - kmap > > > > > + kmap_local_page > > > > > (e2) > > > > > ... when != x = e1 // not stored in any location and not passed to > > > > another function > > > > > when != f(...,e1,...) > > > > > when != x = e2 > > > > > when != f(...,e2,...) > > > > > -kunmap(e2) > > > > > +kunmap_local(e1) > > > > > > > > > > julia > > > > > > > > > > > > > I've never spent sufficient time to understand properly the syntax and > > > > semantics of expressions of Coccinelle. However, thanks Julia, this > > code > > > > looks good and can be very helpful. > > > > > > > > Only a minor objection... it doesn't tell when 'e2' has been allocated > > > > within the same function where the kmap() call is. > > > > > > > > In the particular case that I cite above, I'd prefer to remove the > > > > allocation of the page (say with alloc_page()) and convert kmap() / > > kunmap() > > > > to use kmalloc() / kfree(). > > > > > > > > Fox example, this is done in the following patch: > > > > > > > > commit 633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from > > > > sgx_ioc_enclave_init()") from Ira Weiny. > > > > > > > > Can Coccinelle catch also those special cases where a page that is > > passed > > > > to kmap() is allocated within that same function (vs. being passed as > > > > argument to this function) and, if so, propose a replacement with > > > > kmalloc()? > > > > > > It looks complex in this case, because the allocation is in another > > > function, and it is passed to another function. > > > > This is not the special case I was talking about. In this case your code > > for Coccinelle tells the right proposal and it is exactly what Alaa did in > > her patch (which is good!). > > > > I'm talking about other special cases like the one I pointed to with the > > link I provided. I'm sorry if my bad English made you think that Alaa's > > patch was one of those cases where the page is allocated within the same > > function where kmap() is. > > > > I hope that now I've been clearer :) > > Ah, sorry for the misunderstanding. If you have an example, I can take a > look and propose something for this special case. > > julia Yes, I have the example that you are asking for. It's that commit 633b0616cfe0 from Ira Weiny. Let me copy and paste it here for your convenience... diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ ioctl.c index 90a5caf76939..2e10367ea66c 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -604,7 +604,6 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) { struct sgx_sigstruct *sigstruct; struct sgx_enclave_init init_arg; - struct page *initp_page; void *token; int ret; @@ -615,11 +614,15 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) if (copy_from_user(&init_arg, arg, sizeof(init_arg))) return -EFAULT; - initp_page = alloc_page(GFP_KERNEL); - if (!initp_page) + /* + * 'sigstruct' must be on a page boundary and 'token' on a 512 byte + * boundary. kmalloc() will give this alignment when allocating + * PAGE_SIZE bytes. + */ + sigstruct = kmalloc(PAGE_SIZE, GFP_KERNEL); + if (!sigstruct) return -ENOMEM; - sigstruct = kmap(initp_page); token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2); memset(token, 0, SGX_LAUNCH_TOKEN_SIZE); @@ -645,8 +648,7 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) ret = sgx_encl_init(encl, sigstruct, token); out: - kunmap(initp_page); - __free_page(initp_page); + kfree(sigstruct); return ret; } I think that Coccinelle might understand that "initp_page" is allocated in the same function where later it is kmap()'ed. But I'm not able to write a Coccinelle check to find out these kinds of special cases. In these cases the correct solution is not to use kmap_local_page(). Instead delete the alloc_page() and use kmalloc(). Thanks, Fabio
On 4/19/22 7:43 PM, Alaa Mohamed wrote: > kmap() is being deprecated and these usages are all local to the thread > so there is no reason kmap_local_page() can't be used. > > Replace kmap() calls with kmap_local_page(). > > Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com> Applied to for-linus-5.18. (Juergen, I kept your R-b from v1) -boris
On Wed, Apr 20, 2022 at 04:07:36PM +0200, Fabio M. De Francesco wrote: > On mercoledì 20 aprile 2022 15:57:14 CEST Julia Lawall wrote: > > > > On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > > > > > On mercoledì 20 aprile 2022 15:40:10 CEST Julia Lawall wrote: > > > > > > > > On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > > > > > > > > > On mercoledì 20 aprile 2022 08:03:05 CEST Julia Lawall wrote: > > > > > > > > > > > > On Wed, 20 Apr 2022, Alaa Mohamed wrote: > > > > > > > > > > > > > kmap() is being deprecated and these usages are all local to > the > > > thread > > > > > > > so there is no reason kmap_local_page() can't be used. > > > > > > > > > > > > > > Replace kmap() calls with kmap_local_page(). > > > > > > > > > > > > OK, so from a Coccinelle point of view, could we do > > > > > > > > > > > > @@ > > > > > > expression e1,e2,x,f; > > > > > > @@ > > > > > > > > > > > > e1 = > > > > > > - kmap > > > > > > + kmap_local_page > > > > > > (e2) > > > > > > ... when != x = e1 // not stored in any location and not passed > to > > > > > another function > > > > > > when != f(...,e1,...) > > > > > > when != x = e2 > > > > > > when != f(...,e2,...) > > > > > > -kunmap(e2) > > > > > > +kunmap_local(e1) > > > > > > > > > > > > julia > > > > > > > > > > > > > > > > I've never spent sufficient time to understand properly the syntax > and > > > > > semantics of expressions of Coccinelle. However, thanks Julia, this > > > code > > > > > looks good and can be very helpful. > > > > > > > > > > Only a minor objection... it doesn't tell when 'e2' has been > allocated > > > > > within the same function where the kmap() call is. > > > > > > > > > > In the particular case that I cite above, I'd prefer to remove the > > > > > allocation of the page (say with alloc_page()) and convert kmap() / > > > kunmap() > > > > > to use kmalloc() / kfree(). > > > > > > > > > > Fox example, this is done in the following patch: > > > > > > > > > > commit 633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from > > > > > sgx_ioc_enclave_init()") from Ira Weiny. > > > > > > > > > > Can Coccinelle catch also those special cases where a page that is > > > passed > > > > > to kmap() is allocated within that same function (vs. being passed > as > > > > > argument to this function) and, if so, propose a replacement with > > > > > kmalloc()? > > > > > > > > It looks complex in this case, because the allocation is in another > > > > function, and it is passed to another function. > > > > > > This is not the special case I was talking about. In this case your > code > > > for Coccinelle tells the right proposal and it is exactly what Alaa did > in > > > her patch (which is good!). > > > > > > I'm talking about other special cases like the one I pointed to with > the > > > link I provided. I'm sorry if my bad English made you think that Alaa's > > > patch was one of those cases where the page is allocated within the > same > > > function where kmap() is. > > > > > > I hope that now I've been clearer :) > > > > Ah, sorry for the misunderstanding. If you have an example, I can take a > > look and propose something for this special case. > > > > julia > > Yes, I have the example that you are asking for. It's that commit > 633b0616cfe0 from Ira Weiny. > > Let me copy and paste it here for your convenience... > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ > ioctl.c > index 90a5caf76939..2e10367ea66c 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -604,7 +604,6 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, > void __user *arg) > { > struct sgx_sigstruct *sigstruct; > struct sgx_enclave_init init_arg; > - struct page *initp_page; > void *token; > int ret; > > @@ -615,11 +614,15 @@ static long sgx_ioc_enclave_init(struct sgx_encl > *encl, void __user *arg) > if (copy_from_user(&init_arg, arg, sizeof(init_arg))) > return -EFAULT; > > - initp_page = alloc_page(GFP_KERNEL); > - if (!initp_page) > + /* > + * 'sigstruct' must be on a page boundary and 'token' on a 512 byte > + * boundary. kmalloc() will give this alignment when allocating > + * PAGE_SIZE bytes. > + */ > + sigstruct = kmalloc(PAGE_SIZE, GFP_KERNEL); > + if (!sigstruct) > return -ENOMEM; > > - sigstruct = kmap(initp_page); > token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2); > memset(token, 0, SGX_LAUNCH_TOKEN_SIZE); > > @@ -645,8 +648,7 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, > void __user *arg) > ret = sgx_encl_init(encl, sigstruct, token); > > out: > - kunmap(initp_page); > - __free_page(initp_page); > + kfree(sigstruct); > return ret; > } > > I think that Coccinelle might understand that "initp_page" is allocated in > the same function where later it is kmap()'ed. But I'm not able to write a > Coccinelle check to find out these kinds of special cases. In these cases > the correct solution is not to use kmap_local_page(). Instead delete the > alloc_page() and use kmalloc(). > Sorry about missing this thread last week... I've lost the Coccinelle scripts I wrote before but the ones which helped were documented in patches I submitted when Coccinelle was used. I think Coccinelle can help a lot. And probably a lot more than I know since I'm not an expert in the language either. However, In addition to the example Fabio shows above here are a few other things to look out for when writing Coccinelle scripts. 1) The addition of mem*_page() functions means sometimes the entire kmap/kunmap can be removed. Check out the Coccinelle script for that.[1] 2) kunmap_local() has ordering rules which often requires some manual review.[2] 3) kmap/kunmap is often wrapped in other subsystem helper functions. I was not sure how to deal with that in Coccinelle. Julia is this easy in Coccinelle?[3] Ira [1] https://lore.kernel.org/lkml/20210205232304.1670522-3-ira.weiny@intel.com/ https://lore.kernel.org/lkml/20210205232304.1670522-5-ira.weiny@intel.com/ [2] https://lore.kernel.org/lkml/20210217024826.3466046-3-ira.weiny@intel.com/ https://lore.kernel.org/lkml/20210217024826.3466046-4-ira.weiny@intel.com/ [3] https://lore.kernel.org/lkml/20210217024826.3466046-5-ira.weiny@intel.com/ > > Thanks, > > Fabio > > >
On Mon, 25 Apr 2022, Ira Weiny wrote: > On Wed, Apr 20, 2022 at 04:07:36PM +0200, Fabio M. De Francesco wrote: > > On mercoledì 20 aprile 2022 15:57:14 CEST Julia Lawall wrote: > > > > > > On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > > > > > > > On mercoledì 20 aprile 2022 15:40:10 CEST Julia Lawall wrote: > > > > > > > > > > On Wed, 20 Apr 2022, Fabio M. De Francesco wrote: > > > > > > > > > > > On mercoledì 20 aprile 2022 08:03:05 CEST Julia Lawall wrote: > > > > > > > > > > > > > > On Wed, 20 Apr 2022, Alaa Mohamed wrote: > > > > > > > > > > > > > > > kmap() is being deprecated and these usages are all local to > > the > > > > thread > > > > > > > > so there is no reason kmap_local_page() can't be used. > > > > > > > > > > > > > > > > Replace kmap() calls with kmap_local_page(). > > > > > > > > > > > > > > OK, so from a Coccinelle point of view, could we do > > > > > > > > > > > > > > @@ > > > > > > > expression e1,e2,x,f; > > > > > > > @@ > > > > > > > > > > > > > > e1 = > > > > > > > - kmap > > > > > > > + kmap_local_page > > > > > > > (e2) > > > > > > > ... when != x = e1 // not stored in any location and not passed > > to > > > > > > another function > > > > > > > when != f(...,e1,...) > > > > > > > when != x = e2 > > > > > > > when != f(...,e2,...) > > > > > > > -kunmap(e2) > > > > > > > +kunmap_local(e1) > > > > > > > > > > > > > > julia > > > > > > > > > > > > > > > > > > > I've never spent sufficient time to understand properly the syntax > > and > > > > > > semantics of expressions of Coccinelle. However, thanks Julia, this > > > > code > > > > > > looks good and can be very helpful. > > > > > > > > > > > > Only a minor objection... it doesn't tell when 'e2' has been > > allocated > > > > > > within the same function where the kmap() call is. > > > > > > > > > > > > In the particular case that I cite above, I'd prefer to remove the > > > > > > allocation of the page (say with alloc_page()) and convert kmap() / > > > > kunmap() > > > > > > to use kmalloc() / kfree(). > > > > > > > > > > > > Fox example, this is done in the following patch: > > > > > > > > > > > > commit 633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from > > > > > > sgx_ioc_enclave_init()") from Ira Weiny. > > > > > > > > > > > > Can Coccinelle catch also those special cases where a page that is > > > > passed > > > > > > to kmap() is allocated within that same function (vs. being passed > > as > > > > > > argument to this function) and, if so, propose a replacement with > > > > > > kmalloc()? > > > > > > > > > > It looks complex in this case, because the allocation is in another > > > > > function, and it is passed to another function. > > > > > > > > This is not the special case I was talking about. In this case your > > code > > > > for Coccinelle tells the right proposal and it is exactly what Alaa did > > in > > > > her patch (which is good!). > > > > > > > > I'm talking about other special cases like the one I pointed to with > > the > > > > link I provided. I'm sorry if my bad English made you think that Alaa's > > > > patch was one of those cases where the page is allocated within the > > same > > > > function where kmap() is. > > > > > > > > I hope that now I've been clearer :) > > > > > > Ah, sorry for the misunderstanding. If you have an example, I can take a > > > look and propose something for this special case. > > > > > > julia > > > > Yes, I have the example that you are asking for. It's that commit > > 633b0616cfe0 from Ira Weiny. > > > > Let me copy and paste it here for your convenience... > > > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ > > ioctl.c > > index 90a5caf76939..2e10367ea66c 100644 > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > > @@ -604,7 +604,6 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, > > void __user *arg) > > { > > struct sgx_sigstruct *sigstruct; > > struct sgx_enclave_init init_arg; > > - struct page *initp_page; > > void *token; > > int ret; > > > > @@ -615,11 +614,15 @@ static long sgx_ioc_enclave_init(struct sgx_encl > > *encl, void __user *arg) > > if (copy_from_user(&init_arg, arg, sizeof(init_arg))) > > return -EFAULT; > > > > - initp_page = alloc_page(GFP_KERNEL); > > - if (!initp_page) > > + /* > > + * 'sigstruct' must be on a page boundary and 'token' on a 512 byte > > + * boundary. kmalloc() will give this alignment when allocating > > + * PAGE_SIZE bytes. > > + */ > > + sigstruct = kmalloc(PAGE_SIZE, GFP_KERNEL); > > + if (!sigstruct) > > return -ENOMEM; > > > > - sigstruct = kmap(initp_page); > > token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2); > > memset(token, 0, SGX_LAUNCH_TOKEN_SIZE); > > > > @@ -645,8 +648,7 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, > > void __user *arg) > > ret = sgx_encl_init(encl, sigstruct, token); > > > > out: > > - kunmap(initp_page); > > - __free_page(initp_page); > > + kfree(sigstruct); > > return ret; > > } > > > > I think that Coccinelle might understand that "initp_page" is allocated in > > the same function where later it is kmap()'ed. But I'm not able to write a > > Coccinelle check to find out these kinds of special cases. In these cases > > the correct solution is not to use kmap_local_page(). Instead delete the > > alloc_page() and use kmalloc(). > > > > Sorry about missing this thread last week... > > I've lost the Coccinelle scripts I wrote before but the ones which helped were > documented in patches I submitted when Coccinelle was used. > > I think Coccinelle can help a lot. And probably a lot more than I know since > I'm not an expert in the language either. > > However, In addition to the example Fabio shows above here are a few other > things to look out for when writing Coccinelle scripts. > > 1) The addition of mem*_page() functions means sometimes the entire kmap/kunmap > can be removed. Check out the Coccinelle script for that.[1] > > 2) kunmap_local() has ordering rules which often requires some manual > review.[2] > > 3) kmap/kunmap is often wrapped in other subsystem helper functions. I was not > sure how to deal with that in Coccinelle. Julia is this easy in > Coccinelle?[3] Thanks for the pointers. [3] would depend on how much risk you are willing to take. The arguments are the same, except for the array index, for example. But one may prefer to be sure that nothing complex happens between the two calls. julia > > > Ira > > > [1] > https://lore.kernel.org/lkml/20210205232304.1670522-3-ira.weiny@intel.com/ > https://lore.kernel.org/lkml/20210205232304.1670522-5-ira.weiny@intel.com/ > > [2] > https://lore.kernel.org/lkml/20210217024826.3466046-3-ira.weiny@intel.com/ > https://lore.kernel.org/lkml/20210217024826.3466046-4-ira.weiny@intel.com/ > > [3] > https://lore.kernel.org/lkml/20210217024826.3466046-5-ira.weiny@intel.com/ > > > > > Thanks, > > > > Fabio > > > > > > > >
diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c index 4849f94372a4..55acb32842a3 100644 --- a/drivers/xen/gntalloc.c +++ b/drivers/xen/gntalloc.c @@ -178,9 +178,9 @@ static void __del_gref(struct gntalloc_gref *gref) unsigned long addr; if (gref->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) { - uint8_t *tmp = kmap(gref->page); + uint8_t *tmp = kmap_local_page(gref->page); tmp[gref->notify.pgoff] = 0; - kunmap(gref->page); + kunmap_local(tmp); } if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) { notify_remote_via_evtchn(gref->notify.event);
kmap() is being deprecated and these usages are all local to the thread so there is no reason kmap_local_page() can't be used. Replace kmap() calls with kmap_local_page(). Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com> --- changes in V2: -edit commit subject -edit commit message --- drivers/xen/gntalloc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)