[API-NEXT] api: system_info: add function for fetching all supported huge page sizes

Message ID 1496314359-12839-1-git-send-email-matias.elo@nokia.com
State Superseded
Headers show

Commit Message

Elo, Matias (Nokia - FI/Espoo) June 1, 2017, 10:52 a.m.
A system may simultaneously support multiple huge page sizes. Add a new API
function odp_sys_huge_page_size_all() which returns all supported page
sizes. odp_sys_huge_page_size() stays unmodified to maintain backward
compatibility.

Signed-off-by: Matias Elo <matias.elo@nokia.com>

---
 include/odp/api/spec/system_info.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

-- 
2.7.4

Comments

Bill Fischofer June 1, 2017, 12:44 p.m. | #1
This still needs implementation, validation tests, and doc updates to
be complete.

On Thu, Jun 1, 2017 at 5:52 AM, Matias Elo <matias.elo@nokia.com> wrote:
> A system may simultaneously support multiple huge page sizes. Add a new API

> function odp_sys_huge_page_size_all() which returns all supported page

> sizes. odp_sys_huge_page_size() stays unmodified to maintain backward

> compatibility.

>

> Signed-off-by: Matias Elo <matias.elo@nokia.com>


Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>


> ---

>  include/odp/api/spec/system_info.h | 19 +++++++++++++++++++

>  1 file changed, 19 insertions(+)

>

> diff --git a/include/odp/api/spec/system_info.h b/include/odp/api/spec/system_info.h

> index ca4dcdc..c41d3c5 100644

> --- a/include/odp/api/spec/system_info.h

> +++ b/include/odp/api/spec/system_info.h

> @@ -27,10 +27,29 @@ extern "C" {

>   * Default system huge page size in bytes

>   *

>   * @return Default huge page size in bytes

> + * @retval 0 on no huge pages

>   */

>  uint64_t odp_sys_huge_page_size(void);

>

>  /**

> + * System huge page sizes in bytes

> + *

> + * Returns the number of huge page sizes supported by the system. Outputs up to

> + * 'num' sizes when the 'size' array pointer is not NULL. If return value is

> + * larger than 'num', there are more supported sizes than the function was

> + * allowed to output. If return value (N) is less than 'num', only sizes

> + * [0 ... N-1] have been written. Returned values are ordered from smallest to

> + * largest.

> + *

> + * @param[out] size     Points to an array of huge page sizes for output

> + * @param      num      Maximum number of huge page sizes to output

> + *

> + * @return Number of supported huge page sizes

> + * @retval 0 on no huge pages

> + */

> +unsigned odp_sys_huge_page_size_all(uint64_t size[], unsigned num);

> +

> +/**

>   * Page size in bytes

>   *

>   * @return Page size in bytes

> --

> 2.7.4

>
Elo, Matias (Nokia - FI/Espoo) June 1, 2017, 12:57 p.m. | #2
Thanks, will do.

-Matias

> On 1 Jun 2017, at 15:44, Bill Fischofer <bill.fischofer@linaro.org> wrote:

> 

> This still needs implementation, validation tests, and doc updates to

> be complete.

> 

> On Thu, Jun 1, 2017 at 5:52 AM, Matias Elo <matias.elo@nokia.com> wrote:

>> A system may simultaneously support multiple huge page sizes. Add a new API

>> function odp_sys_huge_page_size_all() which returns all supported page

>> sizes. odp_sys_huge_page_size() stays unmodified to maintain backward

>> compatibility.

>> 

>> Signed-off-by: Matias Elo <matias.elo@nokia.com>

> 

> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

> 

>> ---

>> include/odp/api/spec/system_info.h | 19 +++++++++++++++++++

>> 1 file changed, 19 insertions(+)

>> 

>> diff --git a/include/odp/api/spec/system_info.h b/include/odp/api/spec/system_info.h

>> index ca4dcdc..c41d3c5 100644

>> --- a/include/odp/api/spec/system_info.h

>> +++ b/include/odp/api/spec/system_info.h

>> @@ -27,10 +27,29 @@ extern "C" {

>>  * Default system huge page size in bytes

>>  *

>>  * @return Default huge page size in bytes

>> + * @retval 0 on no huge pages

>>  */

>> uint64_t odp_sys_huge_page_size(void);

>> 

>> /**

>> + * System huge page sizes in bytes

>> + *

>> + * Returns the number of huge page sizes supported by the system. Outputs up to

>> + * 'num' sizes when the 'size' array pointer is not NULL. If return value is

>> + * larger than 'num', there are more supported sizes than the function was

>> + * allowed to output. If return value (N) is less than 'num', only sizes

>> + * [0 ... N-1] have been written. Returned values are ordered from smallest to

>> + * largest.

>> + *

>> + * @param[out] size     Points to an array of huge page sizes for output

>> + * @param      num      Maximum number of huge page sizes to output

>> + *

>> + * @return Number of supported huge page sizes

>> + * @retval 0 on no huge pages

>> + */

>> +unsigned odp_sys_huge_page_size_all(uint64_t size[], unsigned num);

>> +

>> +/**

>>  * Page size in bytes

>>  *

>>  * @return Page size in bytes

>> --

>> 2.7.4

>>
Maxim Uvarov June 1, 2017, 1:33 p.m. | #3
On 06/01/17 13:52, Matias Elo wrote:
> A system may simultaneously support multiple huge page sizes. Add a new API

> function odp_sys_huge_page_size_all() which returns all supported page

> sizes. odp_sys_huge_page_size() stays unmodified to maintain backward

> compatibility.

> 

> Signed-off-by: Matias Elo <matias.elo@nokia.com>

> ---

>  include/odp/api/spec/system_info.h | 19 +++++++++++++++++++

>  1 file changed, 19 insertions(+)

> 

> diff --git a/include/odp/api/spec/system_info.h b/include/odp/api/spec/system_info.h

> index ca4dcdc..c41d3c5 100644

> --- a/include/odp/api/spec/system_info.h

> +++ b/include/odp/api/spec/system_info.h

> @@ -27,10 +27,29 @@ extern "C" {

>   * Default system huge page size in bytes

>   *

>   * @return Default huge page size in bytes

> + * @retval 0 on no huge pages

>   */

>  uint64_t odp_sys_huge_page_size(void);

>  

>  /**

> + * System huge page sizes in bytes

> + *

> + * Returns the number of huge page sizes supported by the system. Outputs up to

> + * 'num' sizes when the 'size' array pointer is not NULL. If return value is

> + * larger than 'num', there are more supported sizes than the function was

> + * allowed to output. If return value (N) is less than 'num', only sizes

> + * [0 ... N-1] have been written. Returned values are ordered from smallest to

> + * largest.

> + *

> + * @param[out] size     Points to an array of huge page sizes for output

> + * @param      num      Maximum number of huge page sizes to output

> + *

> + * @return Number of supported huge page sizes

> + * @retval 0 on no huge pages

> + */

> +unsigned odp_sys_huge_page_size_all(uint64_t size[], unsigned num);

> +


I think it has to be int. -1 on error, 0 - no hp, > 0 pages.
For linux it might be similar to getpagesizes()
https://linux.die.net/man/3/getpagesizes
"""
if pagesizes is NULL and n_elem is 0, then the number of pages the
system supports is returned. Otherwise, pagesizes is filled with at most
n_elem page sizes.
"""


num might be better to set to int also.

For size uint64_t looks like ok.

But why do we need this inside ODP? It time be reasonable to say that
it's number of pages/sizes visible to current ODP instance (i.e. not the
system global.)

Maxim.

> +/**

>   * Page size in bytes

>   *

>   * @return Page size in bytes

>
Elo, Matias (Nokia - FI/Espoo) June 2, 2017, 9:44 a.m. | #4
>> 

>> /**

>> + * System huge page sizes in bytes

>> + *

>> + * Returns the number of huge page sizes supported by the system. Outputs up to

>> + * 'num' sizes when the 'size' array pointer is not NULL. If return value is

>> + * larger than 'num', there are more supported sizes than the function was

>> + * allowed to output. If return value (N) is less than 'num', only sizes

>> + * [0 ... N-1] have been written. Returned values are ordered from smallest to

>> + * largest.

>> + *

>> + * @param[out] size     Points to an array of huge page sizes for output

>> + * @param      num      Maximum number of huge page sizes to output

>> + *

>> + * @return Number of supported huge page sizes

>> + * @retval 0 on no huge pages

>> + */

>> +unsigned odp_sys_huge_page_size_all(uint64_t size[], unsigned num);

>> +

> 

> I think it has to be int. -1 on error, 0 - no hp, > 0 pages.

> For linux it might be similar to getpagesizes()

> https://linux.die.net/man/3/getpagesizes

> """

> if pagesizes is NULL and n_elem is 0, then the number of pages the

> system supports is returned. Otherwise, pagesizes is filled with at most

> n_elem page sizes.

> """

> 


getpagesizes() returns -1 in a case of invalid function arguments. odp_sys_huge_page_size_all() is documented so that the application cannot pass invalid arguments. So an internal error would be the only possibility. I don't see this to be likely as the function is only reading system info.

Adding -1 return value would also increase application complexity as the error return value would require special handling from application.

> 

> But why do we need this inside ODP? It time be reasonable to say that

> it's number of pages/sizes visible to current ODP instance (i.e. not the

> system global.)

> 


A system can simultaneously support multiple huge page sizes and an application may
for example do some alignment decisions based on this information. I found this issues when implementing shm for odp-dpdk and trying to pass the validation tests. This API change enables adding a proper test for odp_shm_info_t.page_size.

-Matias
Maxim Uvarov June 2, 2017, 1:48 p.m. | #5
On 06/02/17 12:44, Elo, Matias (Nokia - FI/Espoo) wrote:
>>>

>>> /**

>>> + * System huge page sizes in bytes

>>> + *

>>> + * Returns the number of huge page sizes supported by the system. Outputs up to

>>> + * 'num' sizes when the 'size' array pointer is not NULL. If return value is

>>> + * larger than 'num', there are more supported sizes than the function was

>>> + * allowed to output. If return value (N) is less than 'num', only sizes

>>> + * [0 ... N-1] have been written. Returned values are ordered from smallest to

>>> + * largest.

>>> + *

>>> + * @param[out] size     Points to an array of huge page sizes for output

>>> + * @param      num      Maximum number of huge page sizes to output

>>> + *

>>> + * @return Number of supported huge page sizes

>>> + * @retval 0 on no huge pages

>>> + */

>>> +unsigned odp_sys_huge_page_size_all(uint64_t size[], unsigned num);

>>> +

>>

>> I think it has to be int. -1 on error, 0 - no hp, > 0 pages.

>> For linux it might be similar to getpagesizes()

>> https://linux.die.net/man/3/getpagesizes

>> """

>> if pagesizes is NULL and n_elem is 0, then the number of pages the

>> system supports is returned. Otherwise, pagesizes is filled with at most

>> n_elem page sizes.

>> """

>>

> 

> getpagesizes() returns -1 in a case of invalid function arguments. odp_sys_huge_page_size_all() is documented so that the application cannot pass invalid arguments. So an internal error would be the only possibility. I don't see this to be likely as the function is only reading system info.

> 

> Adding -1 return value would also increase application complexity as the error return value would require special handling from application.

> 


We have to be consistent with all odp api functions. We do not have
unsigned function, they are int. This function is not fast path so
additional check is ok. And -1 can be returned on permission error to
assess /proc or /sys files for example or any other internal failure.

Maxim.


>>

>> But why do we need this inside ODP? It time be reasonable to say that

>> it's number of pages/sizes visible to current ODP instance (i.e. not the

>> system global.)

>>

> 

> A system can simultaneously support multiple huge page sizes and an application may

> for example do some alignment decisions based on this information. I found this issues when implementing shm for odp-dpdk and trying to pass the validation tests. This API change enables adding a proper test for odp_shm_info_t.page_size.

> 

> -Matias

> 

>
Elo, Matias (Nokia - FI/Espoo) June 5, 2017, 7:11 a.m. | #6
> On 2 Jun 2017, at 16:48, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> 

> On 06/02/17 12:44, Elo, Matias (Nokia - FI/Espoo) wrote:

>>>> 

>>>> /**

>>>> + * System huge page sizes in bytes

>>>> + *

>>>> + * Returns the number of huge page sizes supported by the system. Outputs up to

>>>> + * 'num' sizes when the 'size' array pointer is not NULL. If return value is

>>>> + * larger than 'num', there are more supported sizes than the function was

>>>> + * allowed to output. If return value (N) is less than 'num', only sizes

>>>> + * [0 ... N-1] have been written. Returned values are ordered from smallest to

>>>> + * largest.

>>>> + *

>>>> + * @param[out] size     Points to an array of huge page sizes for output

>>>> + * @param      num      Maximum number of huge page sizes to output

>>>> + *

>>>> + * @return Number of supported huge page sizes

>>>> + * @retval 0 on no huge pages

>>>> + */

>>>> +unsigned odp_sys_huge_page_size_all(uint64_t size[], unsigned num);

>>>> +

>>> 

>>> I think it has to be int. -1 on error, 0 - no hp, > 0 pages.

>>> For linux it might be similar to getpagesizes()

>>> https://linux.die.net/man/3/getpagesizes

>>> """

>>> if pagesizes is NULL and n_elem is 0, then the number of pages the

>>> system supports is returned. Otherwise, pagesizes is filled with at most

>>> n_elem page sizes.

>>> """

>>> 

>> 

>> getpagesizes() returns -1 in a case of invalid function arguments. odp_sys_huge_page_size_all() is documented so that the application cannot pass invalid arguments. So an internal error would be the only possibility. I don't see this to be likely as the function is only reading system info.

>> 

>> Adding -1 return value would also increase application complexity as the error return value would require special handling from application.

>> 

> 

> We have to be consistent with all odp api functions. We do not have

> unsigned function, they are int.


We do have odp_pktio_max_index(), which returns unsigned, and anyway this shouldn't be a reason not to use otherwise valid return value. Regarding consistency, not returning -1 follows the same return value style as the rest of the functions in system_info.h (odp_sys_huge_page_size(), odp_sys_page_size(), odp_sys_cache_line_size()).

> This function is not fast path so

> additional check is ok. And -1 can be returned on permission error to

> assess /proc or /sys files for example or any other internal failure.


From application point of view the outcome is still the same (no huge pages) and returning -1 would make this function inconsistent with the other functions in this module as noted above.

-Matias
Elo, Matias (Nokia - FI/Espoo) July 3, 2017, 12:40 p.m. | #7
Ping.


> On 5 Jun 2017, at 10:11, Elo, Matias (Nokia - FI/Espoo) <matias.elo@nokia.com> wrote:

> 

> 

>> On 2 Jun 2017, at 16:48, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>> 

>> On 06/02/17 12:44, Elo, Matias (Nokia - FI/Espoo) wrote:

>>>>> 

>>>>> /**

>>>>> + * System huge page sizes in bytes

>>>>> + *

>>>>> + * Returns the number of huge page sizes supported by the system. Outputs up to

>>>>> + * 'num' sizes when the 'size' array pointer is not NULL. If return value is

>>>>> + * larger than 'num', there are more supported sizes than the function was

>>>>> + * allowed to output. If return value (N) is less than 'num', only sizes

>>>>> + * [0 ... N-1] have been written. Returned values are ordered from smallest to

>>>>> + * largest.

>>>>> + *

>>>>> + * @param[out] size     Points to an array of huge page sizes for output

>>>>> + * @param      num      Maximum number of huge page sizes to output

>>>>> + *

>>>>> + * @return Number of supported huge page sizes

>>>>> + * @retval 0 on no huge pages

>>>>> + */

>>>>> +unsigned odp_sys_huge_page_size_all(uint64_t size[], unsigned num);

>>>>> +

>>>> 

>>>> I think it has to be int. -1 on error, 0 - no hp, > 0 pages.

>>>> For linux it might be similar to getpagesizes()

>>>> https://linux.die.net/man/3/getpagesizes

>>>> """

>>>> if pagesizes is NULL and n_elem is 0, then the number of pages the

>>>> system supports is returned. Otherwise, pagesizes is filled with at most

>>>> n_elem page sizes.

>>>> """

>>>> 

>>> 

>>> getpagesizes() returns -1 in a case of invalid function arguments. odp_sys_huge_page_size_all() is documented so that the application cannot pass invalid arguments. So an internal error would be the only possibility. I don't see this to be likely as the function is only reading system info.

>>> 

>>> Adding -1 return value would also increase application complexity as the error return value would require special handling from application.

>>> 

>> 

>> We have to be consistent with all odp api functions. We do not have

>> unsigned function, they are int.

> 

> We do have odp_pktio_max_index(), which returns unsigned, and anyway this shouldn't be a reason not to use otherwise valid return value. Regarding consistency, not returning -1 follows the same return value style as the rest of the functions in system_info.h (odp_sys_huge_page_size(), odp_sys_page_size(), odp_sys_cache_line_size()).

> 

>> This function is not fast path so

>> additional check is ok. And -1 can be returned on permission error to

>> assess /proc or /sys files for example or any other internal failure.

> 

> From application point of view the outcome is still the same (no huge pages) and returning -1 would make this function inconsistent with the other functions in this module as noted above.

> 

> -Matias

> 

>
Bill Fischofer July 3, 2017, 12:54 p.m. | #8
On Mon, Jul 3, 2017 at 7:40 AM, Elo, Matias (Nokia - FI/Espoo)
<matias.elo@nokia.com> wrote:
> Ping.


Is the rest of the patch (implementation, validation test updates, doc
updates) in preparation? The API changes have already been reviewed by
both Petri and me.

>

>

>> On 5 Jun 2017, at 10:11, Elo, Matias (Nokia - FI/Espoo) <matias.elo@nokia.com> wrote:

>>

>>

>>> On 2 Jun 2017, at 16:48, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>>>

>>> On 06/02/17 12:44, Elo, Matias (Nokia - FI/Espoo) wrote:

>>>>>>

>>>>>> /**

>>>>>> + * System huge page sizes in bytes

>>>>>> + *

>>>>>> + * Returns the number of huge page sizes supported by the system. Outputs up to

>>>>>> + * 'num' sizes when the 'size' array pointer is not NULL. If return value is

>>>>>> + * larger than 'num', there are more supported sizes than the function was

>>>>>> + * allowed to output. If return value (N) is less than 'num', only sizes

>>>>>> + * [0 ... N-1] have been written. Returned values are ordered from smallest to

>>>>>> + * largest.

>>>>>> + *

>>>>>> + * @param[out] size     Points to an array of huge page sizes for output

>>>>>> + * @param      num      Maximum number of huge page sizes to output

>>>>>> + *

>>>>>> + * @return Number of supported huge page sizes

>>>>>> + * @retval 0 on no huge pages

>>>>>> + */

>>>>>> +unsigned odp_sys_huge_page_size_all(uint64_t size[], unsigned num);

>>>>>> +

>>>>>

>>>>> I think it has to be int. -1 on error, 0 - no hp, > 0 pages.

>>>>> For linux it might be similar to getpagesizes()

>>>>> https://linux.die.net/man/3/getpagesizes

>>>>> """

>>>>> if pagesizes is NULL and n_elem is 0, then the number of pages the

>>>>> system supports is returned. Otherwise, pagesizes is filled with at most

>>>>> n_elem page sizes.

>>>>> """

>>>>>

>>>>

>>>> getpagesizes() returns -1 in a case of invalid function arguments. odp_sys_huge_page_size_all() is documented so that the application cannot pass invalid arguments. So an internal error would be the only possibility. I don't see this to be likely as the function is only reading system info.

>>>>

>>>> Adding -1 return value would also increase application complexity as the error return value would require special handling from application.

>>>>

>>>

>>> We have to be consistent with all odp api functions. We do not have

>>> unsigned function, they are int.

>>

>> We do have odp_pktio_max_index(), which returns unsigned, and anyway this shouldn't be a reason not to use otherwise valid return value. Regarding consistency, not returning -1 follows the same return value style as the rest of the functions in system_info.h (odp_sys_huge_page_size(), odp_sys_page_size(), odp_sys_cache_line_size()).

>>

>>> This function is not fast path so

>>> additional check is ok. And -1 can be returned on permission error to

>>> assess /proc or /sys files for example or any other internal failure.

>>

>> From application point of view the outcome is still the same (no huge pages) and returning -1 would make this function inconsistent with the other functions in this module as noted above.

>>

>> -Matias

>>

>>

>
Elo, Matias (Nokia - FI/Espoo) July 3, 2017, 12:59 p.m. | #9
> On 3 Jul 2017, at 15:54, Bill Fischofer <bill.fischofer@linaro.org> wrote:

> 

> On Mon, Jul 3, 2017 at 7:40 AM, Elo, Matias (Nokia - FI/Espoo)

> <matias.elo@nokia.com> wrote:

>> Ping.

> 

> Is the rest of the patch (implementation, validation test updates, doc

> updates) in preparation? The API changes have already been reviewed by

> both Petri and me.

> 



Maxim had previously some issues with the API change, but if everything is now OK I can
do the actual implementation.

-Matias
Maxim Uvarov July 5, 2017, 7:09 a.m. | #10
Matias,

I would change it from unsigned. That allows to reuse on variable for all
return code.

int ret;

re t=  odp_init_global()
if (ret) ..
ret = odp_packet()...
if (ret)
ret = odp_sys_huge_page_size_all()  <- here in your case we will need
additional cast to unsigned
if (ret) ...
return ret;

On 3 July 2017 at 15:59, Elo, Matias (Nokia - FI/Espoo) <
matias.elo@nokia.com> wrote:

>

> > On 3 Jul 2017, at 15:54, Bill Fischofer <bill.fischofer@linaro.org>

> wrote:

> >

> > On Mon, Jul 3, 2017 at 7:40 AM, Elo, Matias (Nokia - FI/Espoo)

> > <matias.elo@nokia.com> wrote:

> >> Ping.

> >

> > Is the rest of the patch (implementation, validation test updates, doc

> > updates) in preparation? The API changes have already been reviewed by

> > both Petri and me.

> >

>

>

> Maxim had previously some issues with the API change, but if everything is

> now OK I can

> do the actual implementation.

>

> -Matias

>

>
Elo, Matias (Nokia - FI/Espoo) July 5, 2017, 7:37 a.m. | #11
> On 5 Jul 2017, at 10:09, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> 

> Matias,

> 

> I would change it from unsigned. That allows to reuse on variable for all return code.

> 

> int ret;

> 

> re t=  odp_init_global()

> if (ret) ..

> ret = odp_packet()...

> if (ret)

> ret = odp_sys_huge_page_size_all()  <- here in your case we will need additional cast to unsigned 

> if (ret) ...

> return ret;

> 


OK, I'll change the return value and 'num' param to int in V2.

-Matias

Patch

diff --git a/include/odp/api/spec/system_info.h b/include/odp/api/spec/system_info.h
index ca4dcdc..c41d3c5 100644
--- a/include/odp/api/spec/system_info.h
+++ b/include/odp/api/spec/system_info.h
@@ -27,10 +27,29 @@  extern "C" {
  * Default system huge page size in bytes
  *
  * @return Default huge page size in bytes
+ * @retval 0 on no huge pages
  */
 uint64_t odp_sys_huge_page_size(void);
 
 /**
+ * System huge page sizes in bytes
+ *
+ * Returns the number of huge page sizes supported by the system. Outputs up to
+ * 'num' sizes when the 'size' array pointer is not NULL. If return value is
+ * larger than 'num', there are more supported sizes than the function was
+ * allowed to output. If return value (N) is less than 'num', only sizes
+ * [0 ... N-1] have been written. Returned values are ordered from smallest to
+ * largest.
+ *
+ * @param[out] size     Points to an array of huge page sizes for output
+ * @param      num      Maximum number of huge page sizes to output
+ *
+ * @return Number of supported huge page sizes
+ * @retval 0 on no huge pages
+ */
+unsigned odp_sys_huge_page_size_all(uint64_t size[], unsigned num);
+
+/**
  * Page size in bytes
  *
  * @return Page size in bytes