Project

General

Profile

Actions

Feature #7540

open

Fix rule specification models after in Linux 4.4 GFP_WAIT was renamed with GFP_RECLAIM

Added by Evgeny Novikov over 7 years ago. Updated about 7 years ago.

Status:
Open
Priority:
High
Assignee:
-
Category:
Requirement specifications
Target version:
-
Start date:
08/28/2016
Due date:
% Done:

0%

Estimated time:
Published in build:

Description

There are several such rule specifications. With default settings the issue is observed just for linux:block:request.

In addition it will be great to extract all low-level work with GFP flags to the single place, say, linux/ldv/gfp.h.


Files

Actions #1

Updated by Evgeny Novikov over 7 years ago

  • Subject changed from Fix rule specification models that after in Linux 4.4 GFP_WAIT was renamed with GFP_RECLAIM to Fix rule specification models after in Linux 4.4 GFP_WAIT was renamed with GFP_RECLAIM
Actions #2

Updated by Alexey Khoroshilov about 7 years ago

Tested on 2.6.33.20 and 4.11-rc1.

Use 'git am XXX.patch' to apply the patch.

In addition it will be great to extract all low-level work with GFP flags to the single place, say, linux/ldv/gfp.h.

It is not applicable here, since blk_get_request() uses the flags with specific semantics.

Actions #3

Updated by Evgeny Novikov about 7 years ago

  • Status changed from Resolved to Open
  • Priority changed from Urgent to High

Alexey Khoroshilov wrote:

Tested on 2.6.33.20 and 4.11-rc1.

Use 'git am XXX.patch' to apply the patch.

In addition it will be great to extract all low-level work with GFP flags to the single place, say, linux/ldv/gfp.h.

It is not applicable here, since blk_get_request() uses the flags with specific semantics.

Perhaps so, but other rule specifications leveraging GFP_WAIT don't conform Linux 4.4+. The original feature request asks to fix all of them.

Adding support for new versions of the Linux kernel isn't in a first priority issues list. First of all we have to complete the framework and ensure really good quality for already supported versions.

Actions #4

Updated by Evgeny Novikov about 7 years ago

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.

Actions #5

Updated by Alexey Khoroshilov about 7 years ago

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.

Actions #6

Updated by Evgeny Novikov about 7 years ago

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.

Actions #7

Updated by Alexey Khoroshilov about 7 years ago

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.

Actions #8

Updated by Evgeny Novikov about 7 years ago

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.

Actions #9

Updated by Alexey Khoroshilov about 7 years ago

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.

Actions #10

Updated by Evgeny Novikov about 7 years ago

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.

Actions #11

Updated by Alexey Khoroshilov about 7 years ago

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.

Actions #12

Updated by Evgeny Novikov about 7 years ago

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.

Actions

Also available in: Atom PDF