DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] eal/windows: detect insufficient privileges for hugepages
@ 2020-07-07 20:22 Dmitry Kozlyuk
  2020-07-07 23:45 ` Ranjit Menon
  2020-07-08 21:48 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
  0 siblings, 2 replies; 8+ messages in thread
From: Dmitry Kozlyuk @ 2020-07-07 20:22 UTC (permalink / raw)
  To: dev
  Cc: Narcisa Ana Maria Vasile, Ranjit Menon, Pallavi Kadam,
	Tal Shnaiderman, Dmitry Kozlyuk

AdjustTokenPrivileges() succeeds even if no requested privileges have
been granted; this behavior is documented. Check last error code in
addition to return value to detect such case.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 lib/librte_eal/windows/eal_hugepages.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_eal/windows/eal_hugepages.c b/lib/librte_eal/windows/eal_hugepages.c
index 5779cd325..07a5467d0 100644
--- a/lib/librte_eal/windows/eal_hugepages.c
+++ b/lib/librte_eal/windows/eal_hugepages.c
@@ -41,6 +41,10 @@ hugepage_claim_privilege(void)
 		goto exit;
 	}
 
+	/* AdjustTokenPrivileges() may succeed with ERROR_NOT_ALL_ASSIGNED. */
+	if (GetLastError() != ERROR_SUCCESS)
+		goto exit;
+
 	ret = 0;
 
 exit:
-- 
2.25.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/windows: detect insufficient privileges for hugepages
  2020-07-07 20:22 [dpdk-dev] [PATCH] eal/windows: detect insufficient privileges for hugepages Dmitry Kozlyuk
@ 2020-07-07 23:45 ` Ranjit Menon
  2020-07-08  0:20   ` Dmitry Kozlyuk
  2020-07-08 21:48 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
  1 sibling, 1 reply; 8+ messages in thread
From: Ranjit Menon @ 2020-07-07 23:45 UTC (permalink / raw)
  To: Dmitry Kozlyuk, dev
  Cc: Narcisa Ana Maria Vasile, Pallavi Kadam, Tal Shnaiderman


On 7/7/2020 1:22 PM, Dmitry Kozlyuk wrote:
> AdjustTokenPrivileges() succeeds even if no requested privileges have
> been granted; this behavior is documented. Check last error code in
> addition to return value to detect such case.
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>   lib/librte_eal/windows/eal_hugepages.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/lib/librte_eal/windows/eal_hugepages.c b/lib/librte_eal/windows/eal_hugepages.c
> index 5779cd325..07a5467d0 100644
> --- a/lib/librte_eal/windows/eal_hugepages.c
> +++ b/lib/librte_eal/windows/eal_hugepages.c
> @@ -41,6 +41,10 @@ hugepage_claim_privilege(void)
>   		goto exit;
>   	}
>   
> +	/* AdjustTokenPrivileges() may succeed with ERROR_NOT_ALL_ASSIGNED. */
> +	if (GetLastError() != ERROR_SUCCESS)
> +		goto exit;
> +
>   	ret = 0;
>   
>   exit:

Wouldn't this be better if we could print a message here after 
explicitly checking for the ERROR_NOT_ALL_ASSIGNED return value?

Otherwise, the caller simply gets a -1 return value for a failure with 
no message.


ranjit m.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/windows: detect insufficient privileges for hugepages
  2020-07-07 23:45 ` Ranjit Menon
@ 2020-07-08  0:20   ` Dmitry Kozlyuk
  2020-07-08  6:43     ` Tal Shnaiderman
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Kozlyuk @ 2020-07-08  0:20 UTC (permalink / raw)
  To: Ranjit Menon
  Cc: dev, Narcisa Ana Maria Vasile, Pallavi Kadam, Tal Shnaiderman

On Tue, 7 Jul 2020 16:45:07 -0700, Ranjit Menon wrote:
> On 7/7/2020 1:22 PM, Dmitry Kozlyuk wrote:
> > AdjustTokenPrivileges() succeeds even if no requested privileges have
> > been granted; this behavior is documented. Check last error code in
> > addition to return value to detect such case.
> >
> > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > ---
> >   lib/librte_eal/windows/eal_hugepages.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/librte_eal/windows/eal_hugepages.c b/lib/librte_eal/windows/eal_hugepages.c
> > index 5779cd325..07a5467d0 100644
> > --- a/lib/librte_eal/windows/eal_hugepages.c
> > +++ b/lib/librte_eal/windows/eal_hugepages.c
> > @@ -41,6 +41,10 @@ hugepage_claim_privilege(void)
> >   		goto exit;
> >   	}
> >   
> > +	/* AdjustTokenPrivileges() may succeed with ERROR_NOT_ALL_ASSIGNED. */
> > +	if (GetLastError() != ERROR_SUCCESS)
> > +		goto exit;
> > +
> >   	ret = 0;
> >   
> >   exit:  
> 
> Wouldn't this be better if we could print a message here after 
> explicitly checking for the ERROR_NOT_ALL_ASSIGNED return value?
> 
> Otherwise, the caller simply gets a -1 return value for a failure with 
> no message.

Message is printed at ERR level by the caller. There's no context to add here.

-- 
Dmitry Kozlyuk

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/windows: detect insufficient privileges for hugepages
  2020-07-08  0:20   ` Dmitry Kozlyuk
@ 2020-07-08  6:43     ` Tal Shnaiderman
  2020-07-08  7:49       ` Dmitry Kozlyuk
  0 siblings, 1 reply; 8+ messages in thread
From: Tal Shnaiderman @ 2020-07-08  6:43 UTC (permalink / raw)
  To: Dmitry Kozlyuk, Ranjit Menon; +Cc: dev, Narcisa Ana Maria Vasile, Pallavi Kadam

> Subject: Re: [PATCH] eal/windows: detect insufficient privileges for
> hugepages
> 
> On Tue, 7 Jul 2020 16:45:07 -0700, Ranjit Menon wrote:
> > On 7/7/2020 1:22 PM, Dmitry Kozlyuk wrote:
> > > AdjustTokenPrivileges() succeeds even if no requested privileges
> > > have been granted; this behavior is documented. Check last error
> > > code in addition to return value to detect such case.
> > >
> > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > ---
> > >   lib/librte_eal/windows/eal_hugepages.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > >
> > > diff --git a/lib/librte_eal/windows/eal_hugepages.c
> > > b/lib/librte_eal/windows/eal_hugepages.c
> > > index 5779cd325..07a5467d0 100644
> > > --- a/lib/librte_eal/windows/eal_hugepages.c
> > > +++ b/lib/librte_eal/windows/eal_hugepages.c
> > > @@ -41,6 +41,10 @@ hugepage_claim_privilege(void)
> > >   		goto exit;
> > >   	}
> > >
> > > +	/* AdjustTokenPrivileges() may succeed with
> ERROR_NOT_ALL_ASSIGNED. */
> > > +	if (GetLastError() != ERROR_SUCCESS)
> > > +		goto exit;
> > > +
> > >   	ret = 0;
> > >
> > >   exit:
> >
> > Wouldn't this be better if we could print a message here after
> > explicitly checking for the ERROR_NOT_ALL_ASSIGNED return value?
> >
> > Otherwise, the caller simply gets a -1 return value for a failure with
> > no message.
> 
> Message is printed at ERR level by the caller. There's no context to add here.
> 

Tested successfully on Windows server 2019.
 
The output in case of missing privilege is:

EAL: Cannot claim hugepage privilege
EAL: FATAL: Cannot get hugepage information
EAL: Cannot get hugepage information

Do you think this is enough or do we want to give the user further indication that configuration action from his side might be needed? Something alike "Please verify OS Large-Page support is enabled for the current user"

> --
> Dmitry Kozlyuk
Tested-by: Tal Shnaiderman <talshn@mellanox.com>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/windows: detect insufficient privileges for hugepages
  2020-07-08  6:43     ` Tal Shnaiderman
@ 2020-07-08  7:49       ` Dmitry Kozlyuk
  0 siblings, 0 replies; 8+ messages in thread
From: Dmitry Kozlyuk @ 2020-07-08  7:49 UTC (permalink / raw)
  To: Tal Shnaiderman
  Cc: Ranjit Menon, dev, Narcisa Ana Maria Vasile, Pallavi Kadam

On Wed, 8 Jul 2020 06:43:57 +0000, Tal Shnaiderman wrote:
[snip]

Thanks for the testing.

> Do you think this is enough or do we want to give the user further indication that configuration action from his side might be needed? Something alike "Please verify OS Large-Page support is enabled for the current user"

Yes, troubleshooting suggestions would be useful.

From Microsoft "Error Message Guidelines" [1]:

	Avoid the word "please". It can be interpreted to mean that a
	required action is optional.

[1]:
https://docs.microsoft.com/en-us/windows/win32/debug/error-message-guidelines

-- 
Dmitry Kozlyuk

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v2] eal/windows: detect insufficient privileges for hugepages
  2020-07-07 20:22 [dpdk-dev] [PATCH] eal/windows: detect insufficient privileges for hugepages Dmitry Kozlyuk
  2020-07-07 23:45 ` Ranjit Menon
@ 2020-07-08 21:48 ` Dmitry Kozlyuk
  2020-07-08 22:27   ` Ranjit Menon
  1 sibling, 1 reply; 8+ messages in thread
From: Dmitry Kozlyuk @ 2020-07-08 21:48 UTC (permalink / raw)
  To: dev
  Cc: Narcisa Ana Maria Vasile, Ranjit Menon, Pallavi Kadam,
	Tal Shnaiderman, Dmitry Kozlyuk

AdjustTokenPrivileges() succeeds even if no requested privileges have
been granted; this behavior is documented. Check last error code in
addition to return value to detect such case.

Make error messages more specific and add troubleshooting hint.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---

v2:
    Improve error reporting (Tal Shnaiderman).

 lib/librte_eal/windows/eal_hugepages.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/windows/eal_hugepages.c b/lib/librte_eal/windows/eal_hugepages.c
index 5779cd325..7f8a7dfca 100644
--- a/lib/librte_eal/windows/eal_hugepages.c
+++ b/lib/librte_eal/windows/eal_hugepages.c
@@ -41,6 +41,10 @@ hugepage_claim_privilege(void)
 		goto exit;
 	}
 
+	/* AdjustTokenPrivileges() may succeed with ERROR_NOT_ALL_ASSIGNED. */
+	if (GetLastError() != ERROR_SUCCESS)
+		goto exit;
+
 	ret = 0;
 
 exit:
@@ -98,12 +102,14 @@ int
 eal_hugepage_info_init(void)
 {
 	if (hugepage_claim_privilege() < 0) {
-		RTE_LOG(ERR, EAL, "Cannot claim hugepage privilege\n");
+		RTE_LOG(ERR, EAL, "Cannot claim hugepage privilege\n"
+			"Verify that large-page support privilege "
+			"is assigned to the current user\n");
 		return -1;
 	}
 
 	if (hugepage_info_init() < 0) {
-		RTE_LOG(ERR, EAL, "Cannot get hugepage information\n");
+		RTE_LOG(ERR, EAL, "Cannot discover available hugepages\n");
 		return -1;
 	}
 
-- 
2.25.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] eal/windows: detect insufficient privileges for hugepages
  2020-07-08 21:48 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
@ 2020-07-08 22:27   ` Ranjit Menon
  2020-07-10 21:25     ` Thomas Monjalon
  0 siblings, 1 reply; 8+ messages in thread
From: Ranjit Menon @ 2020-07-08 22:27 UTC (permalink / raw)
  To: Dmitry Kozlyuk, dev
  Cc: Narcisa Ana Maria Vasile, Pallavi Kadam, Tal Shnaiderman


On 7/8/2020 2:48 PM, Dmitry Kozlyuk wrote:
> AdjustTokenPrivileges() succeeds even if no requested privileges have
> been granted; this behavior is documented. Check last error code in
> addition to return value to detect such case.
>
> Make error messages more specific and add troubleshooting hint.
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>
> v2:
>      Improve error reporting (Tal Shnaiderman).
>
>   lib/librte_eal/windows/eal_hugepages.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/windows/eal_hugepages.c b/lib/librte_eal/windows/eal_hugepages.c
> index 5779cd325..7f8a7dfca 100644
> --- a/lib/librte_eal/windows/eal_hugepages.c
> +++ b/lib/librte_eal/windows/eal_hugepages.c
> @@ -41,6 +41,10 @@ hugepage_claim_privilege(void)
>   		goto exit;
>   	}
>   
> +	/* AdjustTokenPrivileges() may succeed with ERROR_NOT_ALL_ASSIGNED. */
> +	if (GetLastError() != ERROR_SUCCESS)
> +		goto exit;
> +
>   	ret = 0;
>   
>   exit:
> @@ -98,12 +102,14 @@ int
>   eal_hugepage_info_init(void)
>   {
>   	if (hugepage_claim_privilege() < 0) {
> -		RTE_LOG(ERR, EAL, "Cannot claim hugepage privilege\n");
> +		RTE_LOG(ERR, EAL, "Cannot claim hugepage privilege\n"
> +			"Verify that large-page support privilege "
> +			"is assigned to the current user\n");
>   		return -1;
>   	}
>   
>   	if (hugepage_info_init() < 0) {
> -		RTE_LOG(ERR, EAL, "Cannot get hugepage information\n");
> +		RTE_LOG(ERR, EAL, "Cannot discover available hugepages\n");
>   		return -1;
>   	}
>   


Acked-by: Ranjit Menon <ranjit.menon@intel.com>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] eal/windows: detect insufficient privileges for hugepages
  2020-07-08 22:27   ` Ranjit Menon
@ 2020-07-10 21:25     ` Thomas Monjalon
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Monjalon @ 2020-07-10 21:25 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: dev, Narcisa Ana Maria Vasile, Pallavi Kadam, Tal Shnaiderman,
	Ranjit Menon

09/07/2020 00:27, Ranjit Menon:
> On 7/8/2020 2:48 PM, Dmitry Kozlyuk wrote:
> > AdjustTokenPrivileges() succeeds even if no requested privileges have
> > been granted; this behavior is documented. Check last error code in
> > addition to return value to detect such case.
> >
> > Make error messages more specific and add troubleshooting hint.
> >
> > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
[...]
> >   	if (hugepage_claim_privilege() < 0) {
> > -		RTE_LOG(ERR, EAL, "Cannot claim hugepage privilege\n");
> > +		RTE_LOG(ERR, EAL, "Cannot claim hugepage privilege\n"
> > +			"Verify that large-page support privilege "
> > +			"is assigned to the current user\n");

The error message can be searched in the code,
so it is better not to break lines.

> Acked-by: Ranjit Menon <ranjit.menon@intel.com>

Applied with merged log lines, thanks.




^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-07-10 21:25 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 20:22 [dpdk-dev] [PATCH] eal/windows: detect insufficient privileges for hugepages Dmitry Kozlyuk
2020-07-07 23:45 ` Ranjit Menon
2020-07-08  0:20   ` Dmitry Kozlyuk
2020-07-08  6:43     ` Tal Shnaiderman
2020-07-08  7:49       ` Dmitry Kozlyuk
2020-07-08 21:48 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
2020-07-08 22:27   ` Ranjit Menon
2020-07-10 21:25     ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).