[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 New
Headers show

Commit Message

Matias Elo 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

>
Matias Elo 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

>
Matias Elo 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

> 

>
Matias Elo 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

Patch hide | download patch | download mbox

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