[RFC] api: pool: add threshold limit to the pool

Message ID 1496665637-15790-1-git-send-email-bala.manoharan@linaro.org
State New
Headers show

Commit Message

Balasubramanian Manoharan June 5, 2017, 12:27 p.m.
Adds threshold limit of the pool to the pool parameters.
This threshold limit is a percentage of total size of the pool.

Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org>

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

-- 
1.9.1

Comments

Stanislaw Kardach June 5, 2017, 12:32 p.m. | #1
Best Regards,
Stanislaw Kardach

On 06/05/2017 02:27 PM, Balasubramanian Manoharan wrote:
> Adds threshold limit of the pool to the pool parameters.

> This threshold limit is a percentage of total size of the pool.

> 

> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org>

> ---

>  include/odp/api/spec/pool.h | 32 ++++++++++++++++++++++++++++++++

>  1 file changed, 32 insertions(+)

> 

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

> index 6fc5b6b..1c1ebe4 100644

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

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

> @@ -20,6 +20,7 @@ extern "C" {

>  #endif

>  

>  #include <odp/api/std_types.h>

> +#include <odp/api/support.h>

>  

>  /** @defgroup odp_pool ODP POOL

>   *  Operations on a pool.

> @@ -127,6 +128,9 @@ typedef struct odp_pool_capability_t {

>  		 * The value of zero means that limited only by the available

>  		 * memory size for the pool. */

>  		uint32_t max_uarea_size;

> +

> +		/** Pool Threshold limit support */

> +		odp_support_t pool_threshold_limit;

>  	} pkt;

>  

>  	/** Timeout pool capabilities  */

> @@ -214,6 +218,17 @@ typedef struct odp_pool_param_t {

>  			    defined by pool capability pkt.max_uarea_size.

>  			    Specify as 0 if no user area is needed. */

>  			uint32_t uarea_size;

> +

> +			/** Pool threshold limit in percentage

> +			 *

> +			 * This value denotes the threshold limit of the pool in

> +			 * percentage of the total size of the pool and is used

> +			 * to configure the Random Early Discard (RED).

> +			 * When the number of packets in the pool is greater

> +			 * than this value RED is initiated in the pool and

> +			 * further incoming packets to the pool are dropped in

> +			 * a random sequence. */

> +			uint8_t threshold_limit;

Is threshold limit "a percentage of total size of the pool" (hence I
guess 0-100) or rather an absolute value that triggers RED?

>  		} pkt;

>  

>  		/** Parameters for timeout pools */

> @@ -329,6 +344,23 @@ uint64_t odp_pool_to_u64(odp_pool_t hdl);

>  void odp_pool_param_init(odp_pool_param_t *param);

>  

>  /**

> + * Set threshold limit of the pool

> + *

> + * Set the threshold limit of the pool as a percentage of the total

> + * size of the pool. This value is used to configure Random Early Discard(RED)

> + * parameters of the pool and any incoming packets to the pool after reaching

> + * this threshold is dropped in a random sequence.

> + *

> + * @param pool			Pool handle

> + * @param threshold_limit	Threshold limit of the pool.

> + *

> + * @retval			0 on success

> + * @retval			-1 on failure

> + */

> +

> +int odp_pool_threshold_limit(odp_pool_t pool, uint8_t threshold_limit);

> +

> +/**

>   * @}

>   */

>  

>
Bill Fischofer June 5, 2017, 12:54 p.m. | #2
On Mon, Jun 5, 2017 at 7:32 AM, Stanislaw Kardach <kda@semihalf.com> wrote:
>

>

> Best Regards,

> Stanislaw Kardach

>

> On 06/05/2017 02:27 PM, Balasubramanian Manoharan wrote:

>> Adds threshold limit of the pool to the pool parameters.

>> This threshold limit is a percentage of total size of the pool.

>>

>> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org>

>> ---

>>  include/odp/api/spec/pool.h | 32 ++++++++++++++++++++++++++++++++

>>  1 file changed, 32 insertions(+)

>>

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

>> index 6fc5b6b..1c1ebe4 100644

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

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

>> @@ -20,6 +20,7 @@ extern "C" {

>>  #endif

>>

>>  #include <odp/api/std_types.h>

>> +#include <odp/api/support.h>

>>

>>  /** @defgroup odp_pool ODP POOL

>>   *  Operations on a pool.

>> @@ -127,6 +128,9 @@ typedef struct odp_pool_capability_t {

>>                * The value of zero means that limited only by the available

>>                * memory size for the pool. */

>>               uint32_t max_uarea_size;

>> +

>> +             /** Pool Threshold limit support */

>> +             odp_support_t pool_threshold_limit;

>>       } pkt;

>>

>>       /** Timeout pool capabilities  */

>> @@ -214,6 +218,17 @@ typedef struct odp_pool_param_t {

>>                           defined by pool capability pkt.max_uarea_size.

>>                           Specify as 0 if no user area is needed. */

>>                       uint32_t uarea_size;

>> +

>> +                     /** Pool threshold limit in percentage

>> +                      *

>> +                      * This value denotes the threshold limit of the pool in

>> +                      * percentage of the total size of the pool and is used

>> +                      * to configure the Random Early Discard (RED).

>> +                      * When the number of packets in the pool is greater

>> +                      * than this value RED is initiated in the pool and

>> +                      * further incoming packets to the pool are dropped in

>> +                      * a random sequence. */

>> +                     uint8_t threshold_limit;

> Is threshold limit "a percentage of total size of the pool" (hence I

> guess 0-100) or rather an absolute value that triggers RED?


Do we want to tie this specifically to RED or try to make things more
general? There are many types of flow/congestion control algorithms
that can be used so perhaps that should be better parameterized? Also,
when specifying thresholds it's customary to set high and low
watermarks to provide hysteresis. Some algorithms also need more than
one data point to control the curves, so again this suggests that we
need additional parameterization for this.

>

>>               } pkt;

>>

>>               /** Parameters for timeout pools */

>> @@ -329,6 +344,23 @@ uint64_t odp_pool_to_u64(odp_pool_t hdl);

>>  void odp_pool_param_init(odp_pool_param_t *param);

>>

>>  /**

>> + * Set threshold limit of the pool

>> + *

>> + * Set the threshold limit of the pool as a percentage of the total

>> + * size of the pool. This value is used to configure Random Early Discard(RED)

>> + * parameters of the pool and any incoming packets to the pool after reaching

>> + * this threshold is dropped in a random sequence.

>> + *

>> + * @param pool                       Pool handle

>> + * @param threshold_limit    Threshold limit of the pool.

>> + *

>> + * @retval                   0 on success

>> + * @retval                   -1 on failure

>> + */

>> +

>> +int odp_pool_threshold_limit(odp_pool_t pool, uint8_t threshold_limit);

>> +

>> +/**

>>   * @}

>>   */

>>

>>
Stanislaw Kardach June 5, 2017, 1:12 p.m. | #3
Best Regards,
Stanislaw Kardach

On 06/05/2017 02:54 PM, Bill Fischofer wrote:
> On Mon, Jun 5, 2017 at 7:32 AM, Stanislaw Kardach <kda@semihalf.com> wrote:

>>

>>

>> Best Regards,

>> Stanislaw Kardach

>>

>> On 06/05/2017 02:27 PM, Balasubramanian Manoharan wrote:

>>> Adds threshold limit of the pool to the pool parameters.

>>> This threshold limit is a percentage of total size of the pool.

>>>

>>> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org>

>>> ---

>>>  include/odp/api/spec/pool.h | 32 ++++++++++++++++++++++++++++++++

>>>  1 file changed, 32 insertions(+)

>>>

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

>>> index 6fc5b6b..1c1ebe4 100644

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

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

>>> @@ -20,6 +20,7 @@ extern "C" {

>>>  #endif

>>>

>>>  #include <odp/api/std_types.h>

>>> +#include <odp/api/support.h>

>>>

>>>  /** @defgroup odp_pool ODP POOL

>>>   *  Operations on a pool.

>>> @@ -127,6 +128,9 @@ typedef struct odp_pool_capability_t {

>>>                * The value of zero means that limited only by the available

>>>                * memory size for the pool. */

>>>               uint32_t max_uarea_size;

>>> +

>>> +             /** Pool Threshold limit support */

>>> +             odp_support_t pool_threshold_limit;

>>>       } pkt;

>>>

>>>       /** Timeout pool capabilities  */

>>> @@ -214,6 +218,17 @@ typedef struct odp_pool_param_t {

>>>                           defined by pool capability pkt.max_uarea_size.

>>>                           Specify as 0 if no user area is needed. */

>>>                       uint32_t uarea_size;

>>> +

>>> +                     /** Pool threshold limit in percentage

>>> +                      *

>>> +                      * This value denotes the threshold limit of the pool in

>>> +                      * percentage of the total size of the pool and is used

>>> +                      * to configure the Random Early Discard (RED).

>>> +                      * When the number of packets in the pool is greater

>>> +                      * than this value RED is initiated in the pool and

>>> +                      * further incoming packets to the pool are dropped in

>>> +                      * a random sequence. */

>>> +                     uint8_t threshold_limit;

>> Is threshold limit "a percentage of total size of the pool" (hence I

>> guess 0-100) or rather an absolute value that triggers RED?

> 

> Do we want to tie this specifically to RED or try to make things more

> general? There are many types of flow/congestion control algorithms

> that can be used so perhaps that should be better parameterized? Also,

> when specifying thresholds it's customary to set high and low

> watermarks to provide hysteresis. Some algorithms also need more than

> one data point to control the curves, so again this suggests that we

> need additional parameterization for this.

> 

I'm not disputing the generalization, I've mentioned RED because it's
right there in the comment.
What I meant is whether this is a percentage value or absolute value (as
in number of buffers), because commit message suggested former and this
comments suggest latter with "When the number of packets in the pool is
greater than this value".

Additionally, shouldn't it be "when number of packets in this pool is
_lower_ than this value"? Since otherwise we're saying that congestion
algorithms are to be applied on a full pull.
>>

>>>               } pkt;

>>>

>>>               /** Parameters for timeout pools */

>>> @@ -329,6 +344,23 @@ uint64_t odp_pool_to_u64(odp_pool_t hdl);

>>>  void odp_pool_param_init(odp_pool_param_t *param);

>>>

>>>  /**

>>> + * Set threshold limit of the pool

>>> + *

>>> + * Set the threshold limit of the pool as a percentage of the total

>>> + * size of the pool. This value is used to configure Random Early Discard(RED)

>>> + * parameters of the pool and any incoming packets to the pool after reaching

>>> + * this threshold is dropped in a random sequence.

>>> + *

>>> + * @param pool                       Pool handle

>>> + * @param threshold_limit    Threshold limit of the pool.

>>> + *

>>> + * @retval                   0 on success

>>> + * @retval                   -1 on failure

>>> + */

>>> +

>>> +int odp_pool_threshold_limit(odp_pool_t pool, uint8_t threshold_limit);

>>> +

>>> +/**

>>>   * @}

>>>   */

>>>

>>>
Balasubramanian Manoharan June 5, 2017, 1:13 p.m. | #4
Regards,
Bala


On 5 June 2017 at 18:02, Stanislaw Kardach <kda@semihalf.com> wrote:
>

>

> Best Regards,

> Stanislaw Kardach

>

> On 06/05/2017 02:27 PM, Balasubramanian Manoharan wrote:

>> Adds threshold limit of the pool to the pool parameters.

>> This threshold limit is a percentage of total size of the pool.

>>

>> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org>

>> ---

>>  include/odp/api/spec/pool.h | 32 ++++++++++++++++++++++++++++++++

>>  1 file changed, 32 insertions(+)

>>

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

>> index 6fc5b6b..1c1ebe4 100644

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

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

>> @@ -20,6 +20,7 @@ extern "C" {

>>  #endif

>>

>>  #include <odp/api/std_types.h>

>> +#include <odp/api/support.h>

>>

>>  /** @defgroup odp_pool ODP POOL

>>   *  Operations on a pool.

>> @@ -127,6 +128,9 @@ typedef struct odp_pool_capability_t {

>>                * The value of zero means that limited only by the available

>>                * memory size for the pool. */

>>               uint32_t max_uarea_size;

>> +

>> +             /** Pool Threshold limit support */

>> +             odp_support_t pool_threshold_limit;

>>       } pkt;

>>

>>       /** Timeout pool capabilities  */

>> @@ -214,6 +218,17 @@ typedef struct odp_pool_param_t {

>>                           defined by pool capability pkt.max_uarea_size.

>>                           Specify as 0 if no user area is needed. */

>>                       uint32_t uarea_size;

>> +

>> +                     /** Pool threshold limit in percentage

>> +                      *

>> +                      * This value denotes the threshold limit of the pool in

>> +                      * percentage of the total size of the pool and is used

>> +                      * to configure the Random Early Discard (RED).

>> +                      * When the number of packets in the pool is greater

>> +                      * than this value RED is initiated in the pool and

>> +                      * further incoming packets to the pool are dropped in

>> +                      * a random sequence. */

>> +                     uint8_t threshold_limit;

> Is threshold limit "a percentage of total size of the pool" (hence I

> guess 0-100) or rather an absolute value that triggers RED?


Yes. It is defined as a percentage.
>

>>               } pkt;

>>

>>               /** Parameters for timeout pools */

>> @@ -329,6 +344,23 @@ uint64_t odp_pool_to_u64(odp_pool_t hdl);

>>  void odp_pool_param_init(odp_pool_param_t *param);

>>

>>  /**

>> + * Set threshold limit of the pool

>> + *

>> + * Set the threshold limit of the pool as a percentage of the total

>> + * size of the pool. This value is used to configure Random Early Discard(RED)

>> + * parameters of the pool and any incoming packets to the pool after reaching

>> + * this threshold is dropped in a random sequence.

>> + *

>> + * @param pool                       Pool handle

>> + * @param threshold_limit    Threshold limit of the pool.

>> + *

>> + * @retval                   0 on success

>> + * @retval                   -1 on failure

>> + */

>> +

>> +int odp_pool_threshold_limit(odp_pool_t pool, uint8_t threshold_limit);

>> +

>> +/**

>>   * @}

>>   */

>>

>>
Balasubramanian Manoharan June 5, 2017, 1:21 p.m. | #5
Regards,
Bala


On 5 June 2017 at 18:42, Stanislaw Kardach <kda@semihalf.com> wrote:
>

>

> Best Regards,

> Stanislaw Kardach

>

> On 06/05/2017 02:54 PM, Bill Fischofer wrote:

>> On Mon, Jun 5, 2017 at 7:32 AM, Stanislaw Kardach <kda@semihalf.com> wrote:

>>>

>>>

>>> Best Regards,

>>> Stanislaw Kardach

>>>

>>> On 06/05/2017 02:27 PM, Balasubramanian Manoharan wrote:

>>>> Adds threshold limit of the pool to the pool parameters.

>>>> This threshold limit is a percentage of total size of the pool.

>>>>

>>>> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org>

>>>> ---

>>>>  include/odp/api/spec/pool.h | 32 ++++++++++++++++++++++++++++++++

>>>>  1 file changed, 32 insertions(+)

>>>>

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

>>>> index 6fc5b6b..1c1ebe4 100644

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

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

>>>> @@ -20,6 +20,7 @@ extern "C" {

>>>>  #endif

>>>>

>>>>  #include <odp/api/std_types.h>

>>>> +#include <odp/api/support.h>

>>>>

>>>>  /** @defgroup odp_pool ODP POOL

>>>>   *  Operations on a pool.

>>>> @@ -127,6 +128,9 @@ typedef struct odp_pool_capability_t {

>>>>                * The value of zero means that limited only by the available

>>>>                * memory size for the pool. */

>>>>               uint32_t max_uarea_size;

>>>> +

>>>> +             /** Pool Threshold limit support */

>>>> +             odp_support_t pool_threshold_limit;

>>>>       } pkt;

>>>>

>>>>       /** Timeout pool capabilities  */

>>>> @@ -214,6 +218,17 @@ typedef struct odp_pool_param_t {

>>>>                           defined by pool capability pkt.max_uarea_size.

>>>>                           Specify as 0 if no user area is needed. */

>>>>                       uint32_t uarea_size;

>>>> +

>>>> +                     /** Pool threshold limit in percentage

>>>> +                      *

>>>> +                      * This value denotes the threshold limit of the pool in

>>>> +                      * percentage of the total size of the pool and is used

>>>> +                      * to configure the Random Early Discard (RED).

>>>> +                      * When the number of packets in the pool is greater

>>>> +                      * than this value RED is initiated in the pool and

>>>> +                      * further incoming packets to the pool are dropped in

>>>> +                      * a random sequence. */

>>>> +                     uint8_t threshold_limit;

>>> Is threshold limit "a percentage of total size of the pool" (hence I

>>> guess 0-100) or rather an absolute value that triggers RED?

>>

>> Do we want to tie this specifically to RED or try to make things more

>> general? There are many types of flow/congestion control algorithms

>> that can be used so perhaps that should be better parameterized? Also,

>> when specifying thresholds it's customary to set high and low

>> watermarks to provide hysteresis. Some algorithms also need more than

>> one data point to control the curves, so again this suggests that we

>> need additional parameterization for this.

>>

> I'm not disputing the generalization, I've mentioned RED because it's

> right there in the comment.

> What I meant is whether this is a percentage value or absolute value (as

> in number of buffers), because commit message suggested former and this

> comments suggest latter with "When the number of packets in the pool is

> greater than this value".


So lets say the total size of the packet pool is 1024 segments and the
threshold for starting/ stopping RED is 750 segments then the
threshold limit is set as 75% rather than the absolute value of 750
segments. I will update the commit message to make it more clear.

>

> Additionally, shouldn't it be "when number of packets in this pool is

> _lower_ than this value"? Since otherwise we're saying that congestion

> algorithms are to be applied on a full pull.


I will update the documentation to indicate when RED gets dropped in
the system. The logic is RED is enabled when packets in pool is
greater than threshold and RED is disabled when packets in pool is
less than or equal to threshold limit.

Regards,
Bala
>>>

>>>>               } pkt;

>>>>

>>>>               /** Parameters for timeout pools */

>>>> @@ -329,6 +344,23 @@ uint64_t odp_pool_to_u64(odp_pool_t hdl);

>>>>  void odp_pool_param_init(odp_pool_param_t *param);

>>>>

>>>>  /**

>>>> + * Set threshold limit of the pool

>>>> + *

>>>> + * Set the threshold limit of the pool as a percentage of the total

>>>> + * size of the pool. This value is used to configure Random Early Discard(RED)

>>>> + * parameters of the pool and any incoming packets to the pool after reaching

>>>> + * this threshold is dropped in a random sequence.

>>>> + *

>>>> + * @param pool                       Pool handle

>>>> + * @param threshold_limit    Threshold limit of the pool.

>>>> + *

>>>> + * @retval                   0 on success

>>>> + * @retval                   -1 on failure

>>>> + */

>>>> +

>>>> +int odp_pool_threshold_limit(odp_pool_t pool, uint8_t threshold_limit);

>>>> +

>>>> +/**

>>>>   * @}

>>>>   */

>>>>

>>>>
Balasubramanian Manoharan June 5, 2017, 1:24 p.m. | #6
Regards,
Bala


On 5 June 2017 at 18:24, Bill Fischofer <bill.fischofer@linaro.org> wrote:
> On Mon, Jun 5, 2017 at 7:32 AM, Stanislaw Kardach <kda@semihalf.com> wrote:

>>

>>

>> Best Regards,

>> Stanislaw Kardach

>>

>> On 06/05/2017 02:27 PM, Balasubramanian Manoharan wrote:

>>> Adds threshold limit of the pool to the pool parameters.

>>> This threshold limit is a percentage of total size of the pool.

>>>

>>> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org>

>>> ---

>>>  include/odp/api/spec/pool.h | 32 ++++++++++++++++++++++++++++++++

>>>  1 file changed, 32 insertions(+)

>>>

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

>>> index 6fc5b6b..1c1ebe4 100644

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

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

>>> @@ -20,6 +20,7 @@ extern "C" {

>>>  #endif

>>>

>>>  #include <odp/api/std_types.h>

>>> +#include <odp/api/support.h>

>>>

>>>  /** @defgroup odp_pool ODP POOL

>>>   *  Operations on a pool.

>>> @@ -127,6 +128,9 @@ typedef struct odp_pool_capability_t {

>>>                * The value of zero means that limited only by the available

>>>                * memory size for the pool. */

>>>               uint32_t max_uarea_size;

>>> +

>>> +             /** Pool Threshold limit support */

>>> +             odp_support_t pool_threshold_limit;

>>>       } pkt;

>>>

>>>       /** Timeout pool capabilities  */

>>> @@ -214,6 +218,17 @@ typedef struct odp_pool_param_t {

>>>                           defined by pool capability pkt.max_uarea_size.

>>>                           Specify as 0 if no user area is needed. */

>>>                       uint32_t uarea_size;

>>> +

>>> +                     /** Pool threshold limit in percentage

>>> +                      *

>>> +                      * This value denotes the threshold limit of the pool in

>>> +                      * percentage of the total size of the pool and is used

>>> +                      * to configure the Random Early Discard (RED).

>>> +                      * When the number of packets in the pool is greater

>>> +                      * than this value RED is initiated in the pool and

>>> +                      * further incoming packets to the pool are dropped in

>>> +                      * a random sequence. */

>>> +                     uint8_t threshold_limit;

>> Is threshold limit "a percentage of total size of the pool" (hence I

>> guess 0-100) or rather an absolute value that triggers RED?

>

> Do we want to tie this specifically to RED or try to make things more

> general? There are many types of flow/congestion control algorithms

> that can be used so perhaps that should be better parameterized? Also,

> when specifying thresholds it's customary to set high and low

> watermarks to provide hysteresis. Some algorithms also need more than

> one data point to control the curves, so again this suggests that we

> need additional parameterization for this.


The idea is to have a configuration value for enabling or disabling
Random Early Discard in the packet ingress which could be configured
either in the queue or in pool depending on the implementation. I
actually thought about having a higher /lower watermark but went
against this since the logic I have followed in this proposal is to
have a mechanism to start or stop the RED, IMO a single threshold
value is sufficient and RED gets initiated when packet is greater than
the threshold and is disabled when the packet limit on the pool is
lesser than this threshold.

>

>>

>>>               } pkt;

>>>

>>>               /** Parameters for timeout pools */

>>> @@ -329,6 +344,23 @@ uint64_t odp_pool_to_u64(odp_pool_t hdl);

>>>  void odp_pool_param_init(odp_pool_param_t *param);

>>>

>>>  /**

>>> + * Set threshold limit of the pool

>>> + *

>>> + * Set the threshold limit of the pool as a percentage of the total

>>> + * size of the pool. This value is used to configure Random Early Discard(RED)

>>> + * parameters of the pool and any incoming packets to the pool after reaching

>>> + * this threshold is dropped in a random sequence.

>>> + *

>>> + * @param pool                       Pool handle

>>> + * @param threshold_limit    Threshold limit of the pool.

>>> + *

>>> + * @retval                   0 on success

>>> + * @retval                   -1 on failure

>>> + */

>>> +

>>> +int odp_pool_threshold_limit(odp_pool_t pool, uint8_t threshold_limit);

>>> +

>>> +/**

>>>   * @}

>>>   */

>>>

>>>
Bill Fischofer June 5, 2017, 4:22 p.m. | #7
On Mon, Jun 5, 2017 at 8:24 AM, Bala Manoharan
<bala.manoharan@linaro.org> wrote:
> Regards,

> Bala

>

>

> On 5 June 2017 at 18:24, Bill Fischofer <bill.fischofer@linaro.org> wrote:

>> On Mon, Jun 5, 2017 at 7:32 AM, Stanislaw Kardach <kda@semihalf.com> wrote:

>>>

>>>

>>> Best Regards,

>>> Stanislaw Kardach

>>>

>>> On 06/05/2017 02:27 PM, Balasubramanian Manoharan wrote:

>>>> Adds threshold limit of the pool to the pool parameters.

>>>> This threshold limit is a percentage of total size of the pool.

>>>>

>>>> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org>

>>>> ---

>>>>  include/odp/api/spec/pool.h | 32 ++++++++++++++++++++++++++++++++

>>>>  1 file changed, 32 insertions(+)

>>>>

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

>>>> index 6fc5b6b..1c1ebe4 100644

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

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

>>>> @@ -20,6 +20,7 @@ extern "C" {

>>>>  #endif

>>>>

>>>>  #include <odp/api/std_types.h>

>>>> +#include <odp/api/support.h>

>>>>

>>>>  /** @defgroup odp_pool ODP POOL

>>>>   *  Operations on a pool.

>>>> @@ -127,6 +128,9 @@ typedef struct odp_pool_capability_t {

>>>>                * The value of zero means that limited only by the available

>>>>                * memory size for the pool. */

>>>>               uint32_t max_uarea_size;

>>>> +

>>>> +             /** Pool Threshold limit support */

>>>> +             odp_support_t pool_threshold_limit;

>>>>       } pkt;

>>>>

>>>>       /** Timeout pool capabilities  */

>>>> @@ -214,6 +218,17 @@ typedef struct odp_pool_param_t {

>>>>                           defined by pool capability pkt.max_uarea_size.

>>>>                           Specify as 0 if no user area is needed. */

>>>>                       uint32_t uarea_size;

>>>> +

>>>> +                     /** Pool threshold limit in percentage

>>>> +                      *

>>>> +                      * This value denotes the threshold limit of the pool in

>>>> +                      * percentage of the total size of the pool and is used

>>>> +                      * to configure the Random Early Discard (RED).

>>>> +                      * When the number of packets in the pool is greater

>>>> +                      * than this value RED is initiated in the pool and

>>>> +                      * further incoming packets to the pool are dropped in

>>>> +                      * a random sequence. */

>>>> +                     uint8_t threshold_limit;

>>> Is threshold limit "a percentage of total size of the pool" (hence I

>>> guess 0-100) or rather an absolute value that triggers RED?

>>

>> Do we want to tie this specifically to RED or try to make things more

>> general? There are many types of flow/congestion control algorithms

>> that can be used so perhaps that should be better parameterized? Also,

>> when specifying thresholds it's customary to set high and low

>> watermarks to provide hysteresis. Some algorithms also need more than

>> one data point to control the curves, so again this suggests that we

>> need additional parameterization for this.

>

> The idea is to have a configuration value for enabling or disabling

> Random Early Discard in the packet ingress which could be configured

> either in the queue or in pool depending on the implementation. I

> actually thought about having a higher /lower watermark but went

> against this since the logic I have followed in this proposal is to

> have a mechanism to start or stop the RED, IMO a single threshold

> value is sufficient and RED gets initiated when packet is greater than

> the threshold and is disabled when the packet limit on the pool is

> lesser than this threshold.


A single value may work for basic RED, but if you want to support
other options like PFC then you need more than a single value or else
you wind up "stuttering" around the single value when utilization is
very close to it. That's because PFC doesn't just do something with a
packet--it actively transmits pause frames.

>

>>

>>>

>>>>               } pkt;

>>>>

>>>>               /** Parameters for timeout pools */

>>>> @@ -329,6 +344,23 @@ uint64_t odp_pool_to_u64(odp_pool_t hdl);

>>>>  void odp_pool_param_init(odp_pool_param_t *param);

>>>>

>>>>  /**

>>>> + * Set threshold limit of the pool

>>>> + *

>>>> + * Set the threshold limit of the pool as a percentage of the total

>>>> + * size of the pool. This value is used to configure Random Early Discard(RED)

>>>> + * parameters of the pool and any incoming packets to the pool after reaching

>>>> + * this threshold is dropped in a random sequence.

>>>> + *

>>>> + * @param pool                       Pool handle

>>>> + * @param threshold_limit    Threshold limit of the pool.

>>>> + *

>>>> + * @retval                   0 on success

>>>> + * @retval                   -1 on failure

>>>> + */

>>>> +

>>>> +int odp_pool_threshold_limit(odp_pool_t pool, uint8_t threshold_limit);

>>>> +

>>>> +/**

>>>>   * @}

>>>>   */

>>>>

>>>>
Balasubramanian Manoharan June 5, 2017, 4:49 p.m. | #8
Regards,
Bala


On 5 June 2017 at 21:52, Bill Fischofer <bill.fischofer@linaro.org> wrote:
> On Mon, Jun 5, 2017 at 8:24 AM, Bala Manoharan

> <bala.manoharan@linaro.org> wrote:

>> Regards,

>> Bala

>>

>>

>> On 5 June 2017 at 18:24, Bill Fischofer <bill.fischofer@linaro.org> wrote:

>>> On Mon, Jun 5, 2017 at 7:32 AM, Stanislaw Kardach <kda@semihalf.com> wrote:

>>>>

>>>>

>>>> Best Regards,

>>>> Stanislaw Kardach

>>>>

>>>> On 06/05/2017 02:27 PM, Balasubramanian Manoharan wrote:

>>>>> Adds threshold limit of the pool to the pool parameters.

>>>>> This threshold limit is a percentage of total size of the pool.

>>>>>

>>>>> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org>

>>>>> ---

>>>>>  include/odp/api/spec/pool.h | 32 ++++++++++++++++++++++++++++++++

>>>>>  1 file changed, 32 insertions(+)

>>>>>

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

>>>>> index 6fc5b6b..1c1ebe4 100644

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

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

>>>>> @@ -20,6 +20,7 @@ extern "C" {

>>>>>  #endif

>>>>>

>>>>>  #include <odp/api/std_types.h>

>>>>> +#include <odp/api/support.h>

>>>>>

>>>>>  /** @defgroup odp_pool ODP POOL

>>>>>   *  Operations on a pool.

>>>>> @@ -127,6 +128,9 @@ typedef struct odp_pool_capability_t {

>>>>>                * The value of zero means that limited only by the available

>>>>>                * memory size for the pool. */

>>>>>               uint32_t max_uarea_size;

>>>>> +

>>>>> +             /** Pool Threshold limit support */

>>>>> +             odp_support_t pool_threshold_limit;

>>>>>       } pkt;

>>>>>

>>>>>       /** Timeout pool capabilities  */

>>>>> @@ -214,6 +218,17 @@ typedef struct odp_pool_param_t {

>>>>>                           defined by pool capability pkt.max_uarea_size.

>>>>>                           Specify as 0 if no user area is needed. */

>>>>>                       uint32_t uarea_size;

>>>>> +

>>>>> +                     /** Pool threshold limit in percentage

>>>>> +                      *

>>>>> +                      * This value denotes the threshold limit of the pool in

>>>>> +                      * percentage of the total size of the pool and is used

>>>>> +                      * to configure the Random Early Discard (RED).

>>>>> +                      * When the number of packets in the pool is greater

>>>>> +                      * than this value RED is initiated in the pool and

>>>>> +                      * further incoming packets to the pool are dropped in

>>>>> +                      * a random sequence. */

>>>>> +                     uint8_t threshold_limit;

>>>> Is threshold limit "a percentage of total size of the pool" (hence I

>>>> guess 0-100) or rather an absolute value that triggers RED?

>>>

>>> Do we want to tie this specifically to RED or try to make things more

>>> general? There are many types of flow/congestion control algorithms

>>> that can be used so perhaps that should be better parameterized? Also,

>>> when specifying thresholds it's customary to set high and low

>>> watermarks to provide hysteresis. Some algorithms also need more than

>>> one data point to control the curves, so again this suggests that we

>>> need additional parameterization for this.

>>

>> The idea is to have a configuration value for enabling or disabling

>> Random Early Discard in the packet ingress which could be configured

>> either in the queue or in pool depending on the implementation. I

>> actually thought about having a higher /lower watermark but went

>> against this since the logic I have followed in this proposal is to

>> have a mechanism to start or stop the RED, IMO a single threshold

>> value is sufficient and RED gets initiated when packet is greater than

>> the threshold and is disabled when the packet limit on the pool is

>> lesser than this threshold.

>

> A single value may work for basic RED, but if you want to support

> other options like PFC then you need more than a single value or else

> you wind up "stuttering" around the single value when utilization is

> very close to it. That's because PFC doesn't just do something with a

> packet--it actively transmits pause frames.


I have only targeted basic RED in this proposal, which is common
across all platforms and it would be great to incorporate it to the
ODP ASAP.
If some other platforms comes with support for PFC at the hardware
level then we can add those additional features.
IMO we could keep this proposal simple so it is easier for upstreaming.

>

>>

>>>

>>>>

>>>>>               } pkt;

>>>>>

>>>>>               /** Parameters for timeout pools */

>>>>> @@ -329,6 +344,23 @@ uint64_t odp_pool_to_u64(odp_pool_t hdl);

>>>>>  void odp_pool_param_init(odp_pool_param_t *param);

>>>>>

>>>>>  /**

>>>>> + * Set threshold limit of the pool

>>>>> + *

>>>>> + * Set the threshold limit of the pool as a percentage of the total

>>>>> + * size of the pool. This value is used to configure Random Early Discard(RED)

>>>>> + * parameters of the pool and any incoming packets to the pool after reaching

>>>>> + * this threshold is dropped in a random sequence.

>>>>> + *

>>>>> + * @param pool                       Pool handle

>>>>> + * @param threshold_limit    Threshold limit of the pool.

>>>>> + *

>>>>> + * @retval                   0 on success

>>>>> + * @retval                   -1 on failure

>>>>> + */

>>>>> +

>>>>> +int odp_pool_threshold_limit(odp_pool_t pool, uint8_t threshold_limit);

>>>>> +

>>>>> +/**

>>>>>   * @}

>>>>>   */

>>>>>

>>>>>

Patch

diff --git a/include/odp/api/spec/pool.h b/include/odp/api/spec/pool.h
index 6fc5b6b..1c1ebe4 100644
--- a/include/odp/api/spec/pool.h
+++ b/include/odp/api/spec/pool.h
@@ -20,6 +20,7 @@  extern "C" {
 #endif
 
 #include <odp/api/std_types.h>
+#include <odp/api/support.h>
 
 /** @defgroup odp_pool ODP POOL
  *  Operations on a pool.
@@ -127,6 +128,9 @@  typedef struct odp_pool_capability_t {
 		 * The value of zero means that limited only by the available
 		 * memory size for the pool. */
 		uint32_t max_uarea_size;
+
+		/** Pool Threshold limit support */
+		odp_support_t pool_threshold_limit;
 	} pkt;
 
 	/** Timeout pool capabilities  */
@@ -214,6 +218,17 @@  typedef struct odp_pool_param_t {
 			    defined by pool capability pkt.max_uarea_size.
 			    Specify as 0 if no user area is needed. */
 			uint32_t uarea_size;
+
+			/** Pool threshold limit in percentage
+			 *
+			 * This value denotes the threshold limit of the pool in
+			 * percentage of the total size of the pool and is used
+			 * to configure the Random Early Discard (RED).
+			 * When the number of packets in the pool is greater
+			 * than this value RED is initiated in the pool and
+			 * further incoming packets to the pool are dropped in
+			 * a random sequence. */
+			uint8_t threshold_limit;
 		} pkt;
 
 		/** Parameters for timeout pools */
@@ -329,6 +344,23 @@  uint64_t odp_pool_to_u64(odp_pool_t hdl);
 void odp_pool_param_init(odp_pool_param_t *param);
 
 /**
+ * Set threshold limit of the pool
+ *
+ * Set the threshold limit of the pool as a percentage of the total
+ * size of the pool. This value is used to configure Random Early Discard(RED)
+ * parameters of the pool and any incoming packets to the pool after reaching
+ * this threshold is dropped in a random sequence.
+ *
+ * @param pool			Pool handle
+ * @param threshold_limit	Threshold limit of the pool.
+ *
+ * @retval			0 on success
+ * @retval			-1 on failure
+ */
+
+int odp_pool_threshold_limit(odp_pool_t pool, uint8_t threshold_limit);
+
+/**
  * @}
  */