Klever

Indeed corresponding rule specifications need refactoring since they use different ways to refer to so called might sleep memory allocation. It does have sense to do refactoring first and this issue second.

Evgeny Novikov wrote:

Indeed corresponding rule specifications need refactoring since they use different ways to refer to so called might sleep memory allocation. It does have sense to do refactoring first and this issue second.

I do not understand what do you mean by "this issue", but first of all you have to apply this patch because it makes current situation better and then you can make any improvements you want.

Instead introducing this dependency on the kernel API in 3-4 places I suggest to define it in linux/ldv/gfp.h. While particular rule specifications should include this header and implement their magic around the common LDV API.

Evgeny Novikov wrote:

Instead introducing this dependency on the kernel API in 3-4 places I suggest to define it in linux/ldv/gfp.h. While particular rule specifications should include this header and implement their magic around the common LDV API.

The idea is good, but it is not well suited for the blk_get_request() case. This function has its own interpretation of low-level gfp flags and it is not reasonable to have this specific interpretation in generic linux/ldv/gfp.h.

Alexey Khoroshilov wrote:

Evgeny Novikov wrote:

Instead introducing this dependency on the kernel API in 3-4 places I suggest to define it in linux/ldv/gfp.h. While particular rule specifications should include this header and implement their magic around the common LDV API.

The idea is good, but it is not well suited for the blk_get_request() case. This function has its own interpretation of low-level gfp flags and it is not reasonable to have this specific interpretation in generic linux/ldv/gfp.h.

But it does have sense to have a high level interface for GFP_WAIT, say, LDV_GFP_WAIT there. While rule specifications have to implement just their specific checks.

In accordance with your logic CHECK_WAIT_FLAGS should be moved from linux/ldv/gfp.h to, say, linux/alloc/common.aspect, since this macrofunction is suitable just for a couple of rule specifications from linux/alloc.

Evgeny Novikov wrote:

Alexey Khoroshilov wrote:

Evgeny Novikov wrote:

Instead introducing this dependency on the kernel API in 3-4 places I suggest to define it in linux/ldv/gfp.h. While particular rule specifications should include this header and implement their magic around the common LDV API.

The idea is good, but it is not well suited for the blk_get_request() case. This function has its own interpretation of low-level gfp flags and it is not reasonable to have this specific interpretation in generic linux/ldv/gfp.h.

But it does have sense to have a high level interface for GFP_WAIT, say, LDV_GFP_WAIT there. While rule specifications have to implement just their specific checks.

In accordance with your logic CHECK_WAIT_FLAGS should be moved from linux/ldv/gfp.h to, say, linux/alloc/common.aspect, since this macrofunction is suitable just for a couple of rule specifications from linux/alloc.

No. CHECK_WAIT_FLAGS is about core semantics of gfp flags themselves and it is reasonable to have it defined in linux/ldv/gfp.h.
By the the way, more appropriate name would be CHECK_GFPFLAGS_MIGHT_SLEEP.

I would not made LDV_GFP_WAIT a part of public interface of linux/ldv/gfp.h, since it does not provide any value in addition to CHECK_GFPFLAGS_MIGHT_SLEEP, which has well-defined semantics. Other potential users have to think what semantics they want first of all.

As for header files, I believe it make sense to move a declaration to a header file if there are or obviously there should be several users of the declaration. Otherwise, it is better to have definition and usage in one file.

Alexey Khoroshilov wrote:

Evgeny Novikov wrote:

Alexey Khoroshilov wrote:

Evgeny Novikov wrote:

Instead introducing this dependency on the kernel API in 3-4 places I suggest to define it in linux/ldv/gfp.h. While particular rule specifications should include this header and implement their magic around the common LDV API.

The idea is good, but it is not well suited for the blk_get_request() case. This function has its own interpretation of low-level gfp flags and it is not reasonable to have this specific interpretation in generic linux/ldv/gfp.h.

But it does have sense to have a high level interface for GFP_WAIT, say, LDV_GFP_WAIT there. While rule specifications have to implement just their specific checks.

In accordance with your logic CHECK_WAIT_FLAGS should be moved from linux/ldv/gfp.h to, say, linux/alloc/common.aspect, since this macrofunction is suitable just for a couple of rule specifications from linux/alloc.

No. CHECK_WAIT_FLAGS is about core semantics of gfp flags themselves and it is reasonable to have it defined in linux/ldv/gfp.h.
By the the way, more appropriate name would be CHECK_GFPFLAGS_MIGHT_SLEEP.

But this core semantics is used just only by a couple of rule specifications located at the same place. So properly put this definition near them. If there will be more users than of course this can be changed.

I would not made LDV_GFP_WAIT a part of public interface of linux/ldv/gfp.h, since it does not provide any value in addition to CHECK_GFPFLAGS_MIGHT_SLEEP, which has well-defined semantics. Other potential users have to think what semantics they want first of all.

LDV_GFP_WAIT will allow all users to refer to GFP_WAIT without introducing dependency on the kernel API (like in your patch).

As for header files, I believe it make sense to move a declaration to a header file if there are or obviously there should be several users of the declaration. Otherwise, it is better to have definition and usage in one file.

So even now there will be 3-4 rule specifications located in different places which will leverage LDV_GFP_WAIT. In addition all of them will use their own semantics over it.

LDV_GFP_WAIT will allow all users to refer to GFP_WAIT without introducing dependency on the kernel API (like in your patch).

I do not understand what are you speaking about. All the rule specs depend on the kernel API by definition.

As for header files, I believe it make sense to move a declaration to a header file if there are or obviously there should be several users of the declaration. Otherwise, it is better to have definition and usage in one file.

So even now there will be 3-4 rule specifications located in different places which will leverage LDV_GFP_WAIT. In addition all of them will use their own semantics over it.

If you read the patch carefully, you will see that there are two places where __GFP_WAIT(old flag) was used in the spec, while two different flags are used for newer kernels. LDV_GFP_WAIT does not help here. If we want to have universal aliases than aliases with fine grained semantics are preferable and GFP_WAIT is not the case.

CHECK_GFPFLAGS_MIGHT_SLEEP should be used in all other places as far as I suspect.
So I do not see any rule specification that can be leveraged by LDV_GFP_WAIT.

Alexey Khoroshilov wrote:

LDV_GFP_WAIT will allow all users to refer to GFP_WAIT without introducing dependency on the kernel API (like in your patch).

I do not understand what are you speaking about. All the rule specs depend on the kernel API by definition.

If we will introduce some high level interface like CHECK_WAIT_FLAGS/_CHECK_GFPFLAGS_MIGHT_SLEEP_ then rule specifications will use it rather than directly refer to the kernel API. This has sense when different rule specifications need this.

As for header files, I believe it make sense to move a declaration to a header file if there are or obviously there should be several users of the declaration. Otherwise, it is better to have definition and usage in one file.

So even now there will be 3-4 rule specifications located in different places which will leverage LDV_GFP_WAIT. In addition all of them will use their own semantics over it.

If you read the patch carefully, you will see that there are two places where __GFP_WAIT(old flag) was used in the spec, while two different flags are used for newer kernels. LDV_GFP_WAIT does not help here. If we want to have universal aliases than aliases with fine grained semantics are preferable and GFP_WAIT is not the case.

CHECK_GFPFLAGS_MIGHT_SLEEP should be used in all other places as far as I suspect.
So I do not see any rule specification that can be leveraged by LDV_GFP_WAIT.

Actually both before and after this patch an approximation without bitwise supports doesn't look well for both old and new kernels. For instance, why it doesn't check flags for equality to GFP_NOFS? Perhaps this isn't possible/used in practice, but this is another rule. Here it is important just that there is some particular bit(s) are set.

I suppose first of all to understand whether it has any sense to check these rules without bitwise support (another issue). Most likely it doesn't and we don't need to spend any time for supporting incorrect approximations. Next for this issue we need to understand what analog of GFP_WAIT is in the Linux 4.4+. I see many different flags instead and it isn't obvious what flags imply atomicity. After all it will become clear whether it has sense to define some high and medium level API for corresponding rule specifications.