| Message ID | 20220204064754.425963-2-frank.heimes@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hipersocket page allocation failure on Ubuntu 20.04 based SSC environments (LP: 1959529) | expand |
On 04/02/2022 07:47, frank.heimes@canonical.com wrote: > From: Julian Wiedmann <jwi@linux.ibm.com> > > BugLink: https://bugs.launchpad.net/bugs/1959529 > > Use dev_alloc_page() for backing the RX buffers with pages. This way we > pick up __GFP_MEMALLOC. > > Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (backported from commit 714c9108851743bb718fbc1bfb81290f12a53854) > Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> > Signed-off-by: Frank Heimes <frank.heimes@canonical.com> > --- > drivers/s390/net/qeth_core_main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c > index ec8c7a640d9e..61372e5c279b 100644 > --- a/drivers/s390/net/qeth_core_main.c > +++ b/drivers/s390/net/qeth_core_main.c > @@ -227,7 +227,7 @@ static int qeth_alloc_buffer_pool(struct qeth_card *card) > return -ENOMEM; > } > for (j = 0; j < QETH_MAX_BUFFER_ELEMENTS(card); ++j) { > - ptr = (void *) __get_free_page(GFP_KERNEL); > + ptr = (void *) __dev_alloc_page(GFP_KERNEL); This does not look correct. New API returns "struct page*", previous returns mapped page address. These are not the same. Either we need to backport f81649dfa5343eef7e579eb6f8dd8bd6d300ec31 first or a page_address dance like: struct page *page = __dev_alloc_page(GFP_KERNEL); if (!page) // handle -ENOMEM ptr = page_address(page); > if (!ptr) { > while (j > 0) > free_page((unsigned long) > @@ -2612,7 +2612,7 @@ static struct qeth_buffer_pool_entry *qeth_find_free_buffer_pool_entry( > struct qeth_buffer_pool_entry, list); > for (i = 0; i < QETH_MAX_BUFFER_ELEMENTS(card); ++i) { > if (page_count(virt_to_page(entry->elements[i])) > 1) { > - page = alloc_page(GFP_ATOMIC); > + page = dev_alloc_page(); > if (!page) { > return NULL; > } else { Best regards, Krzysztof
Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
On Fri, Feb 04, 2022 at 10:29:21AM +0100, Krzysztof Kozlowski wrote: > On 04/02/2022 07:47, frank.heimes@canonical.com wrote: > > From: Julian Wiedmann <jwi@linux.ibm.com> > > > > BugLink: https://bugs.launchpad.net/bugs/1959529 > > > > Use dev_alloc_page() for backing the RX buffers with pages. This way we > > pick up __GFP_MEMALLOC. > > > > Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > (backported from commit 714c9108851743bb718fbc1bfb81290f12a53854) > > Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> > > Signed-off-by: Frank Heimes <frank.heimes@canonical.com> > > --- > > drivers/s390/net/qeth_core_main.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c > > index ec8c7a640d9e..61372e5c279b 100644 > > --- a/drivers/s390/net/qeth_core_main.c > > +++ b/drivers/s390/net/qeth_core_main.c > > @@ -227,7 +227,7 @@ static int qeth_alloc_buffer_pool(struct qeth_card *card) > > return -ENOMEM; > > } > > for (j = 0; j < QETH_MAX_BUFFER_ELEMENTS(card); ++j) { > > - ptr = (void *) __get_free_page(GFP_KERNEL); > > + ptr = (void *) __dev_alloc_page(GFP_KERNEL); > Or since the whole point is to use __GPF_MEMALLOC, simply do: + ptr = (void *) __get_free_page(GFP_KERNEL|__GFP_MEMALLOC); > This does not look correct. New API returns "struct page*", previous > returns mapped page address. These are not the same. > > Either we need to backport f81649dfa5343eef7e579eb6f8dd8bd6d300ec31 > first or a page_address dance like: > > struct page *page = __dev_alloc_page(GFP_KERNEL); > if (!page) > // handle -ENOMEM > ptr = page_address(page); > > > > if (!ptr) { > > while (j > 0) > > free_page((unsigned long) > > @@ -2612,7 +2612,7 @@ static struct qeth_buffer_pool_entry *qeth_find_free_buffer_pool_entry( > > struct qeth_buffer_pool_entry, list); > > for (i = 0; i < QETH_MAX_BUFFER_ELEMENTS(card); ++i) { > > if (page_count(virt_to_page(entry->elements[i])) > 1) { > > - page = alloc_page(GFP_ATOMIC); > > + page = dev_alloc_page(); > > if (!page) { > > return NULL; > > } else { > > > Best regards, > Krzysztof > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 04/02/2022 14:49, Thadeu Lima de Souza Cascardo wrote: > On Fri, Feb 04, 2022 at 10:29:21AM +0100, Krzysztof Kozlowski wrote: >> On 04/02/2022 07:47, frank.heimes@canonical.com wrote: >>> From: Julian Wiedmann <jwi@linux.ibm.com> >>> >>> BugLink: https://bugs.launchpad.net/bugs/1959529 >>> >>> Use dev_alloc_page() for backing the RX buffers with pages. This way we >>> pick up __GFP_MEMALLOC. >>> >>> Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com> >>> Signed-off-by: David S. Miller <davem@davemloft.net> >>> (backported from commit 714c9108851743bb718fbc1bfb81290f12a53854) >>> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> >>> Signed-off-by: Frank Heimes <frank.heimes@canonical.com> >>> --- >>> drivers/s390/net/qeth_core_main.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c >>> index ec8c7a640d9e..61372e5c279b 100644 >>> --- a/drivers/s390/net/qeth_core_main.c >>> +++ b/drivers/s390/net/qeth_core_main.c >>> @@ -227,7 +227,7 @@ static int qeth_alloc_buffer_pool(struct qeth_card *card) >>> return -ENOMEM; >>> } >>> for (j = 0; j < QETH_MAX_BUFFER_ELEMENTS(card); ++j) { >>> - ptr = (void *) __get_free_page(GFP_KERNEL); >>> + ptr = (void *) __dev_alloc_page(GFP_KERNEL); >> > > Or since the whole point is to use __GPF_MEMALLOC, simply do: > It is not exact equivalent of __dev_alloc_page (__get_free_page() picks closest NUMA node, __dev_alloc_page() loads entire NUMA policy and chooses node according to it) , but I guess the differences here do not matter. The MEMALLOC, which allows using emergency memory pools and should not be used outside of memory management subsystem or outside paths freeing memory, seems a weird solution to reported problem of lack of memory. The Bug report is saying that memory allocation failed, without too many details so probably because of lack of memory in the system. :) This is a RX buffer. Allocating a generic network RX buffer in case of out-of-memory situation from emergency pools does not fit into MEMALLOC purpose... However if IBM feels it is proper approach for their problem, let's go with simply MEMALLOC flag, like Thadeu suggested. Best regards, Krzysztof
On 04/02/2022 15:29, Krzysztof Kozlowski wrote: > On 04/02/2022 14:49, Thadeu Lima de Souza Cascardo wrote: >> On Fri, Feb 04, 2022 at 10:29:21AM +0100, Krzysztof Kozlowski wrote: >>> On 04/02/2022 07:47, frank.heimes@canonical.com wrote: >>>> From: Julian Wiedmann <jwi@linux.ibm.com> >>>> >>>> BugLink: https://bugs.launchpad.net/bugs/1959529 >>>> >>>> Use dev_alloc_page() for backing the RX buffers with pages. This way we >>>> pick up __GFP_MEMALLOC. >>>> >>>> Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com> >>>> Signed-off-by: David S. Miller <davem@davemloft.net> >>>> (backported from commit 714c9108851743bb718fbc1bfb81290f12a53854) >>>> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> >>>> Signed-off-by: Frank Heimes <frank.heimes@canonical.com> >>>> --- >>>> drivers/s390/net/qeth_core_main.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c >>>> index ec8c7a640d9e..61372e5c279b 100644 >>>> --- a/drivers/s390/net/qeth_core_main.c >>>> +++ b/drivers/s390/net/qeth_core_main.c >>>> @@ -227,7 +227,7 @@ static int qeth_alloc_buffer_pool(struct qeth_card *card) >>>> return -ENOMEM; >>>> } >>>> for (j = 0; j < QETH_MAX_BUFFER_ELEMENTS(card); ++j) { >>>> - ptr = (void *) __get_free_page(GFP_KERNEL); >>>> + ptr = (void *) __dev_alloc_page(GFP_KERNEL); >>> >> >> Or since the whole point is to use __GPF_MEMALLOC, simply do: >> > > It is not exact equivalent of __dev_alloc_page (__get_free_page() picks > closest NUMA node, __dev_alloc_page() loads entire NUMA policy and > chooses node according to it) , but I guess the differences here do not > matter. > > The MEMALLOC, which allows using emergency memory pools and should not > be used outside of memory management subsystem or outside paths freeing > memory, seems a weird solution to reported problem of lack of memory. > The Bug report is saying that memory allocation failed, without too many > details so probably because of lack of memory in the system. :) This is > a RX buffer. Allocating a generic network RX buffer in case of > out-of-memory situation from emergency pools does not fit into MEMALLOC > purpose... > > However if IBM feels it is proper approach for their problem, let's go > with simply MEMALLOC flag, like Thadeu suggested. > Alexandra Winter from IBM proposed to pick up only second part of the patch which looks correct. This fixes their issue, see: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1959529/comments/7 However picking up only half of a stable commit will confuse some stable scripts (e.g. stable-commit-in-tree from stable-tools). Since we skip parts of the commit, I propose to bring it as "UBUNTU: SAUCE:" patch by us, with a similar title/explanation, but not exactly the same. Any comments on such approach? Best regards, Krzysztof
On Mon, Feb 07, 2022 at 04:15:04PM +0100, Krzysztof Kozlowski wrote: > On 04/02/2022 15:29, Krzysztof Kozlowski wrote: > > On 04/02/2022 14:49, Thadeu Lima de Souza Cascardo wrote: > >> On Fri, Feb 04, 2022 at 10:29:21AM +0100, Krzysztof Kozlowski wrote: > >>> On 04/02/2022 07:47, frank.heimes@canonical.com wrote: > >>>> From: Julian Wiedmann <jwi@linux.ibm.com> > >>>> > >>>> BugLink: https://bugs.launchpad.net/bugs/1959529 > >>>> > >>>> Use dev_alloc_page() for backing the RX buffers with pages. This way we > >>>> pick up __GFP_MEMALLOC. > >>>> > >>>> Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com> > >>>> Signed-off-by: David S. Miller <davem@davemloft.net> > >>>> (backported from commit 714c9108851743bb718fbc1bfb81290f12a53854) > >>>> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> > >>>> Signed-off-by: Frank Heimes <frank.heimes@canonical.com> > >>>> --- > >>>> drivers/s390/net/qeth_core_main.c | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c > >>>> index ec8c7a640d9e..61372e5c279b 100644 > >>>> --- a/drivers/s390/net/qeth_core_main.c > >>>> +++ b/drivers/s390/net/qeth_core_main.c > >>>> @@ -227,7 +227,7 @@ static int qeth_alloc_buffer_pool(struct qeth_card *card) > >>>> return -ENOMEM; > >>>> } > >>>> for (j = 0; j < QETH_MAX_BUFFER_ELEMENTS(card); ++j) { > >>>> - ptr = (void *) __get_free_page(GFP_KERNEL); > >>>> + ptr = (void *) __dev_alloc_page(GFP_KERNEL); > >>> > >> > >> Or since the whole point is to use __GPF_MEMALLOC, simply do: > >> > > > > It is not exact equivalent of __dev_alloc_page (__get_free_page() picks > > closest NUMA node, __dev_alloc_page() loads entire NUMA policy and > > chooses node according to it) , but I guess the differences here do not > > matter. > > > > The MEMALLOC, which allows using emergency memory pools and should not > > be used outside of memory management subsystem or outside paths freeing > > memory, seems a weird solution to reported problem of lack of memory. > > The Bug report is saying that memory allocation failed, without too many > > details so probably because of lack of memory in the system. :) This is > > a RX buffer. Allocating a generic network RX buffer in case of > > out-of-memory situation from emergency pools does not fit into MEMALLOC > > purpose... > > > > However if IBM feels it is proper approach for their problem, let's go > > with simply MEMALLOC flag, like Thadeu suggested. > > > > Alexandra Winter from IBM proposed to pick up only second part of the > patch which looks correct. This fixes their issue, see: > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1959529/comments/7 > > However picking up only half of a stable commit will confuse some stable > scripts (e.g. stable-commit-in-tree from stable-tools). > > Since we skip parts of the commit, I propose to bring it as "UBUNTU: > SAUCE:" patch by us, with a similar title/explanation, but not exactly > the same. > > Any comments on such approach? > > Best regards, > Krzysztof I would rather backport the other hunk too, by just adding the MEMALLOC flag. Cascardo.
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c index ec8c7a640d9e..61372e5c279b 100644 --- a/drivers/s390/net/qeth_core_main.c +++ b/drivers/s390/net/qeth_core_main.c @@ -227,7 +227,7 @@ static int qeth_alloc_buffer_pool(struct qeth_card *card) return -ENOMEM; } for (j = 0; j < QETH_MAX_BUFFER_ELEMENTS(card); ++j) { - ptr = (void *) __get_free_page(GFP_KERNEL); + ptr = (void *) __dev_alloc_page(GFP_KERNEL); if (!ptr) { while (j > 0) free_page((unsigned long) @@ -2612,7 +2612,7 @@ static struct qeth_buffer_pool_entry *qeth_find_free_buffer_pool_entry( struct qeth_buffer_pool_entry, list); for (i = 0; i < QETH_MAX_BUFFER_ELEMENTS(card); ++i) { if (page_count(virt_to_page(entry->elements[i])) > 1) { - page = alloc_page(GFP_ATOMIC); + page = dev_alloc_page(); if (!page) { return NULL; } else {