diff mbox

[API-NEXT,PATCHv2] api: pktio link

Message ID 1444739969-7936-1-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov Oct. 13, 2015, 12:39 p.m. UTC
Define API to get pktio link state: seed, autoneg, link up/down,
duplex, started/stopped state.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 v2: - use simple struct to return pktio link state;
     - odp will not modify link, only ready it's state;


 include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Bill Fischofer Oct. 13, 2015, 12:52 p.m. UTC | #1
Is there a reason we want to aggregate this into a struct?  These are
independent bits of metadata and should have their own access functions for
best application/implementation flexibility.

On Tue, Oct 13, 2015 at 7:39 AM, Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:

> Define API to get pktio link state: seed, autoneg, link up/down,
> duplex, started/stopped state.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  v2: - use simple struct to return pktio link state;
>      - odp will not modify link, only ready it's state;
>
>
>  include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
> index d8e69ed..6c77e8d 100644
> --- a/include/odp/api/packet_io.h
> +++ b/include/odp/api/packet_io.h
> @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
>  } odp_pktio_param_t;
>
>  /**
> + * Packet IO link state information
> + */
> +typedef struct odp_pktio_link_state_t {
> +       uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000 etc */
> +       odp_bool_t up;          /**< 1 - link up, 0 - link down */
> +       odp_bool_t autoneg;     /**< 1 - autoneg on, 0 - off */
> +       odp_bool_t fd;          /**< 1 - full duplex, 0 - half duplex */
> +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
> +} odp_pktio_link_state_t;
> +
> +/**
> + * Get packet IO link state
> + *
> + * @param[in] pktio    Packet IO handle
> + * @param[out] state   Buffer to save link state
> + *
> + * @retval 0 on success (state info updated)
> + * @retval <0 on failure (state info not updated)
> + */
> +int odp_pktio_link_state(odp_pktio_t pktio, odp_pktio_link_state_t
> *state);
> +
> +/**
>   * Open a packet IO interface
>   *
>   * An ODP program can open a single packet IO interface per device,
> attempts
> --
> 1.9.1
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
Maxim Uvarov Oct. 14, 2015, 10:36 a.m. UTC | #2
On 10/13/2015 15:52, Bill Fischofer wrote:
> Is there a reason we want to aggregate this into a struct?  These are 
> independent bits of metadata and should have their own access 
> functions for best application/implementation flexibility.
>

it was 2 separate function in my v1 patch. After that propose was to 
keep it simple and do one struct, so it's  that v2 patch. There were no 
objections to do that.

I think that link state are not performance functions and used only for 
debug propose. Like - no packets for some time, let's check if cable is 
plugged or not.
Link speed set up is also related to control plane,  and data plane will 
read it probably only once. I still think it's good to aggregate it to 
one call.

Maxim.

> On Tue, Oct 13, 2015 at 7:39 AM, Maxim Uvarov <maxim.uvarov@linaro.org 
> <mailto:maxim.uvarov@linaro.org>> wrote:
>
>     Define API to get pktio link state: seed, autoneg, link up/down,
>     duplex, started/stopped state.
>
>     Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>     <mailto:maxim.uvarov@linaro.org>>
>     ---
>      v2: - use simple struct to return pktio link state;
>          - odp will not modify link, only ready it's state;
>
>
>      include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
>      1 file changed, 22 insertions(+)
>
>     diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
>     index d8e69ed..6c77e8d 100644
>     --- a/include/odp/api/packet_io.h
>     +++ b/include/odp/api/packet_io.h
>     @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
>      } odp_pktio_param_t;
>
>      /**
>     + * Packet IO link state information
>     + */
>     +typedef struct odp_pktio_link_state_t {
>     +       uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000
>     etc */
>     +       odp_bool_t up;          /**< 1 - link up, 0 - link down */
>     +       odp_bool_t autoneg;     /**< 1 - autoneg on, 0 - off */
>     +       odp_bool_t fd;          /**< 1 - full duplex, 0 - half
>     duplex */
>     +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
>     +} odp_pktio_link_state_t;
>     +
>     +/**
>     + * Get packet IO link state
>     + *
>     + * @param[in] pktio    Packet IO handle
>     + * @param[out] state   Buffer to save link state
>     + *
>     + * @retval 0 on success (state info updated)
>     + * @retval <0 on failure (state info not updated)
>     + */
>     +int odp_pktio_link_state(odp_pktio_t pktio,
>     odp_pktio_link_state_t *state);
>     +
>     +/**
>       * Open a packet IO interface
>       *
>       * An ODP program can open a single packet IO interface per
>     device, attempts
>     --
>     1.9.1
>
>     _______________________________________________
>     lng-odp mailing list
>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>     https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Bill Fischofer Oct. 14, 2015, 11:43 a.m. UTC | #3
Perhaps we can discuss this more during today's ARCH call.  ODP uses
structs for consolidated parameter info (e.g., odp_pool_param_t,
odp_queue_param_t) and these are retuned in corresponding info calls,
however metadata is manipulated via individual accessor functions since the
layout of metadata is implementation-dependent.

On Wed, Oct 14, 2015 at 5:36 AM, Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:

> On 10/13/2015 15:52, Bill Fischofer wrote:
>
>> Is there a reason we want to aggregate this into a struct?  These are
>> independent bits of metadata and should have their own access functions for
>> best application/implementation flexibility.
>>
>>
> it was 2 separate function in my v1 patch. After that propose was to keep
> it simple and do one struct, so it's  that v2 patch. There were no
> objections to do that.
>
> I think that link state are not performance functions and used only for
> debug propose. Like - no packets for some time, let's check if cable is
> plugged or not.
> Link speed set up is also related to control plane,  and data plane will
> read it probably only once. I still think it's good to aggregate it to one
> call.
>
> Maxim.
>
> On Tue, Oct 13, 2015 at 7:39 AM, Maxim Uvarov <maxim.uvarov@linaro.org
>> <mailto:maxim.uvarov@linaro.org>> wrote:
>>
>>     Define API to get pktio link state: seed, autoneg, link up/down,
>>     duplex, started/stopped state.
>>
>>     Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>>     <mailto:maxim.uvarov@linaro.org>>
>>
>>     ---
>>      v2: - use simple struct to return pktio link state;
>>          - odp will not modify link, only ready it's state;
>>
>>
>>      include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
>>      1 file changed, 22 insertions(+)
>>
>>     diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
>>     index d8e69ed..6c77e8d 100644
>>     --- a/include/odp/api/packet_io.h
>>     +++ b/include/odp/api/packet_io.h
>>     @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
>>      } odp_pktio_param_t;
>>
>>      /**
>>     + * Packet IO link state information
>>     + */
>>     +typedef struct odp_pktio_link_state_t {
>>     +       uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000
>>     etc */
>>     +       odp_bool_t up;          /**< 1 - link up, 0 - link down */
>>     +       odp_bool_t autoneg;     /**< 1 - autoneg on, 0 - off */
>>     +       odp_bool_t fd;          /**< 1 - full duplex, 0 - half
>>     duplex */
>>     +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
>>     +} odp_pktio_link_state_t;
>>     +
>>     +/**
>>     + * Get packet IO link state
>>     + *
>>     + * @param[in] pktio    Packet IO handle
>>     + * @param[out] state   Buffer to save link state
>>     + *
>>     + * @retval 0 on success (state info updated)
>>     + * @retval <0 on failure (state info not updated)
>>     + */
>>     +int odp_pktio_link_state(odp_pktio_t pktio,
>>     odp_pktio_link_state_t *state);
>>     +
>>     +/**
>>       * Open a packet IO interface
>>       *
>>       * An ODP program can open a single packet IO interface per
>>     device, attempts
>>     --
>>     1.9.1
>>
>>     _______________________________________________
>>     lng-odp mailing list
>>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>     https://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>
>
Ola Liljedahl Oct. 15, 2015, 8:12 p.m. UTC | #4
On 13 October 2015 at 08:39, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> Define API to get pktio link state: seed, autoneg, link up/down,
> duplex, started/stopped state.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  v2: - use simple struct to return pktio link state;
>      - odp will not modify link, only ready it's state;
>
>
>  include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
> index d8e69ed..6c77e8d 100644
> --- a/include/odp/api/packet_io.h
> +++ b/include/odp/api/packet_io.h
> @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
>  } odp_pktio_param_t;
>
>  /**
> + * Packet IO link state information
> + */
> +typedef struct odp_pktio_link_state_t {
> +       uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000 etc */
> +       odp_bool_t up;          /**< 1 - link up, 0 - link down */
> +       odp_bool_t autoneg;     /**< 1 - autoneg on, 0 - off */
>
Is autonegotiation enabled/disabled of any interest to the application?
It's a configuration input, not an output or result.

+       odp_bool_t fd;          /**< 1 - full duplex, 0 - half duplex */
> +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
>
Link state, link speed and duplex mode all related to the physical state of
the pktio interface. "stopped" is a logical concept of the ODP pktio
abstraction (equivalent to interface up/down?) and there is no one-to-one
correspondence between pktio instances and physical links (some pktio
classes may not even have a physical link). Seems strange to mix those
concepts. Also "stopped' is a poor name me thinks, why not "running"?


> +} odp_pktio_link_state_t;
> +
> +/**
> + * Get packet IO link state
> + *
> + * @param[in] pktio    Packet IO handle
> + * @param[out] state   Buffer to save link state
> + *
> + * @retval 0 on success (state info updated)
> + * @retval <0 on failure (state info not updated)
> + */
> +int odp_pktio_link_state(odp_pktio_t pktio, odp_pktio_link_state_t
> *state);
> +
> +/**
>   * Open a packet IO interface
>   *
>   * An ODP program can open a single packet IO interface per device,
> attempts
> --
> 1.9.1
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
Brian Brooks Oct. 16, 2015, 3:06 a.m. UTC | #5
On Tue, Oct 13, 2015 at 03:39:29PM +0300, Maxim Uvarov wrote:
> Define API to get pktio link state: seed, autoneg, link up/down,
> duplex, started/stopped state.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  v2: - use simple struct to return pktio link state;
>      - odp will not modify link, only ready it's state;
>
>
>  include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
> index d8e69ed..6c77e8d 100644
> --- a/include/odp/api/packet_io.h
> +++ b/include/odp/api/packet_io.h
> @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
>  } odp_pktio_param_t;
>
>  /**
> + * Packet IO link state information
> + */
> +typedef struct odp_pktio_link_state_t {
> +     uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000 etc */
> +     odp_bool_t up;          /**< 1 - link up, 0 - link down */
> +     odp_bool_t autoneg;     /**< 1 - autoneg on, 0 - off */
> +     odp_bool_t fd;          /**< 1 - full duplex, 0 - half duplex */

Since AN and duplex are physical layer dependent, should the API simply be
just link state (up/down) and speed?

Letting physical layer dependent information creep into a generic 'link'
struct might result in unnatural situations where a user is wondering about
AN and duplex for 40GbE or Link Fault Signaling for 1GbE.

> +     odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
> +} odp_pktio_link_state_t;
> +
> +/**
> + * Get packet IO link state
> + *
> + * @param[in] pktio  Packet IO handle
> + * @param[out] state Buffer to save link state
> + *
> + * @retval 0 on success (state info updated)
> + * @retval <0 on failure (state info not updated)
> + */
> +int odp_pktio_link_state(odp_pktio_t pktio, odp_pktio_link_state_t *state);
> +
> +/**
>   * Open a packet IO interface
>   *
>   * An ODP program can open a single packet IO interface per device, attempts
> --
> 1.9.1
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
Savolainen, Petri (Nokia - FI/Espoo) Oct. 16, 2015, 10:24 a.m. UTC | #6
RFC 3635 helps also here to define properties of an Ethernet-like interface. It's not so as crucial to follow RFC terms and definitions here as it's with counters, but we can use it as a guide line. Addresses and promisc mode are classification issues. Name and pktio status (stopped / started) ( == ifAdminStatus) are pktio level data.


typedef struct odp_pktio_link_state_t {
	uint8_t  type;        // 0: Ethernet (or enum ODP_PKTIO_ETHERNET), ...
	uint8_t  status;      // 1: link up, 0: down, -1 not_present (== ifOperStatus )
	uint8_t  autoneg;     // 1: autoneg on, 0: off, -1: not supported
	uint8_t  duplex;      // 1: full duplex, 0: half duplex
	uint32_t speed_mbps;  // Speed in Mbps. This leaves room for
				   // speed_bps which would support odd (Mbps) link speeds
	uint32_t mtu;         // MTU in bytes
} odp_pktio_link_state_t;


Better to use int (uint8_t) than bool to enable extendable status definitions. E.g. ifOperStatus == not_present means that some HW component is missing, rather than user have configured link down.


-Petri



> -----Original Message-----

> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT

> Maxim Uvarov

> Sent: Tuesday, October 13, 2015 3:39 PM

> To: lng-odp@lists.linaro.org

> Subject: [lng-odp] [API-NEXT PATCHv2] api: pktio link

> 

> Define API to get pktio link state: seed, autoneg, link up/down,

> duplex, started/stopped state.

> 

> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>

> ---

>  v2: - use simple struct to return pktio link state;

>      - odp will not modify link, only ready it's state;

> 

> 

>  include/odp/api/packet_io.h | 22 ++++++++++++++++++++++

>  1 file changed, 22 insertions(+)

> 

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

> index d8e69ed..6c77e8d 100644

> --- a/include/odp/api/packet_io.h

> +++ b/include/odp/api/packet_io.h

> @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {

>  } odp_pktio_param_t;

> 

>  /**

> + * Packet IO link state information

> + */

> +typedef struct odp_pktio_link_state_t {

> +	uint32_t speed;		/**< Speed in Mbps: 10, 100, 1000 etc */

> +	odp_bool_t up;		/**< 1 - link up, 0 - link down */

> +	odp_bool_t autoneg;	/**< 1 - autoneg on, 0 - off */

> +	odp_bool_t fd;		/**< 1 - full duplex, 0 - half duplex */

> +	odp_bool_t stopped;	/**< 1 - pktio stopped, 0 - started */

> +} odp_pktio_link_state_t;

> +

> +/**

> + * Get packet IO link state

> + *

> + * @param[in] pktio	Packet IO handle

> + * @param[out] state	Buffer to save link state

> + *

> + * @retval 0 on success (state info updated)

> + * @retval <0 on failure (state info not updated)

> + */

> +int odp_pktio_link_state(odp_pktio_t pktio, odp_pktio_link_state_t

> *state);

> +

> +/**

>   * Open a packet IO interface

>   *

>   * An ODP program can open a single packet IO interface per device,

> attempts

> --

> 1.9.1

> 

> _______________________________________________

> lng-odp mailing list

> lng-odp@lists.linaro.org

> https://lists.linaro.org/mailman/listinfo/lng-odp
Bill Fischofer Oct. 16, 2015, 10:43 a.m. UTC | #7
If these fields are to support -1 values, shouldn't they be int8_t, not
uint8_t?  Also, RFC 3635 does not define a struct, but rather the various
fields that comprise the MIB.

On Fri, Oct 16, 2015 at 5:24 AM, Savolainen, Petri (Nokia - FI/Espoo) <
petri.savolainen@nokia.com> wrote:

>
> RFC 3635 helps also here to define properties of an Ethernet-like
> interface. It's not so as crucial to follow RFC terms and definitions here
> as it's with counters, but we can use it as a guide line. Addresses and
> promisc mode are classification issues. Name and pktio status (stopped /
> started) ( == ifAdminStatus) are pktio level data.
>
>
> typedef struct odp_pktio_link_state_t {
>         uint8_t  type;        // 0: Ethernet (or enum ODP_PKTIO_ETHERNET),
> ...
>         uint8_t  status;      // 1: link up, 0: down, -1 not_present (==
> ifOperStatus )
>         uint8_t  autoneg;     // 1: autoneg on, 0: off, -1: not supported
>         uint8_t  duplex;      // 1: full duplex, 0: half duplex
>         uint32_t speed_mbps;  // Speed in Mbps. This leaves room for
>                                    // speed_bps which would support odd
> (Mbps) link speeds
>         uint32_t mtu;         // MTU in bytes
> } odp_pktio_link_state_t;
>
>
> Better to use int (uint8_t) than bool to enable extendable status
> definitions. E.g. ifOperStatus == not_present means that some HW component
> is missing, rather than user have configured link down.
>
>
> -Petri
>
>
>
> > -----Original Message-----
> > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT
> > Maxim Uvarov
> > Sent: Tuesday, October 13, 2015 3:39 PM
> > To: lng-odp@lists.linaro.org
> > Subject: [lng-odp] [API-NEXT PATCHv2] api: pktio link
> >
> > Define API to get pktio link state: seed, autoneg, link up/down,
> > duplex, started/stopped state.
> >
> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > ---
> >  v2: - use simple struct to return pktio link state;
> >      - odp will not modify link, only ready it's state;
> >
> >
> >  include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
> > index d8e69ed..6c77e8d 100644
> > --- a/include/odp/api/packet_io.h
> > +++ b/include/odp/api/packet_io.h
> > @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
> >  } odp_pktio_param_t;
> >
> >  /**
> > + * Packet IO link state information
> > + */
> > +typedef struct odp_pktio_link_state_t {
> > +     uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000 etc */
> > +     odp_bool_t up;          /**< 1 - link up, 0 - link down */
> > +     odp_bool_t autoneg;     /**< 1 - autoneg on, 0 - off */
> > +     odp_bool_t fd;          /**< 1 - full duplex, 0 - half duplex */
> > +     odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
> > +} odp_pktio_link_state_t;
> > +
> > +/**
> > + * Get packet IO link state
> > + *
> > + * @param[in] pktio  Packet IO handle
> > + * @param[out] state Buffer to save link state
> > + *
> > + * @retval 0 on success (state info updated)
> > + * @retval <0 on failure (state info not updated)
> > + */
> > +int odp_pktio_link_state(odp_pktio_t pktio, odp_pktio_link_state_t
> > *state);
> > +
> > +/**
> >   * Open a packet IO interface
> >   *
> >   * An ODP program can open a single packet IO interface per device,
> > attempts
> > --
> > 1.9.1
> >
> > _______________________________________________
> > lng-odp mailing list
> > lng-odp@lists.linaro.org
> > https://lists.linaro.org/mailman/listinfo/lng-odp
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
Maxim Uvarov Oct. 16, 2015, 3:55 p.m. UTC | #8
On 10/16/2015 13:24, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> RFC 3635 helps also here to define properties of an Ethernet-like interface. It's not so as crucial to follow RFC terms and definitions here as it's with counters, but we can use it as a guide line. Addresses and promisc mode are classification issues. Name and pktio status (stopped / started) ( == ifAdminStatus) are pktio level data.
>
>
> typedef struct odp_pktio_link_state_t {
> 	uint8_t  type;        // 0: Ethernet (or enum ODP_PKTIO_ETHERNET), ...

type is not state.  I think there might be odp_pktio_info_t which is 
filled by init of each pktio.
and some call like:
  const odp_pktio_info_t * odp_pktio_info(pktio);

where
odp_pktio_info_t {

char *name;
uint8_t  type;        // 0: Ethernet (or enum ODP_PKTIO_ETHERNET)

// what also do we need, i/o address space, number of hw queues, nodes 
and etc.
}

But it looks like Bill right pointed that for now we have separate call 
for each function.
We already have mtu and name for example. So we need to understand if we 
are going
to modify existence API or add new.

Also I can understand that:
1. speed -  might be needed to set up some timeout setting.
2.  status link up/down and logical pktio started / stopped is needed to 
check why odp_schedule() returns timeouts instead of packets.

But I have no idea why application needs type, autoneg and duplex.

I think for first try we need to stay just with:
+typedef struct odp_pktio_link_state_t {
+       uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000 etc */
+       odp_bool_t up;          /**< 1 - link up, 0 - link down */
+       odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
+} odp_pktio_link_state_t;

I want to have up and stopped in one struct to speed that data plane calls.
Most likely it will be the following check

ev = odp_schedule(wait_time);
if (ev == ODP_EVENT_INVALID) {
    odp_pktio_link_state(pktio, &link_state);
    if (!link_state.up || link_state.stopped) {
                 <send something to control plane,  >
                   <or just wait for link, or pktio started>
                   continue;
     } else {
         <just timeout, packet can be in any moment, ready to get them>
         continue;
   }
}

I.e. try to not do 2 calls and roll stack for 2 calls.

Does that reasonable?

Maxim.


> 	uint8_t  status;      // 1: link up, 0: down, -1 not_present (== ifOperStatus )
> 	uint8_t  autoneg;     // 1: autoneg on, 0: off, -1: not supported
> 	uint8_t  duplex;      // 1: full duplex, 0: half duplex
> 	uint32_t speed_mbps;  // Speed in Mbps. This leaves room for
> 				   // speed_bps which would support odd (Mbps) link speeds
> 	uint32_t mtu;         // MTU in bytes
> } odp_pktio_link_state_t;
>
>
> Better to use int (uint8_t) than bool to enable extendable status definitions. E.g. ifOperStatus == not_present means that some HW component is missing, rather than user have configured link down.
>
>
> -Petri
>
>
>
>> -----Original Message-----
>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT
>> Maxim Uvarov
>> Sent: Tuesday, October 13, 2015 3:39 PM
>> To: lng-odp@lists.linaro.org
>> Subject: [lng-odp] [API-NEXT PATCHv2] api: pktio link
>>
>> Define API to get pktio link state: seed, autoneg, link up/down,
>> duplex, started/stopped state.
>>
>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>> ---
>>   v2: - use simple struct to return pktio link state;
>>       - odp will not modify link, only ready it's state;
>>
>>
>>   include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
>> index d8e69ed..6c77e8d 100644
>> --- a/include/odp/api/packet_io.h
>> +++ b/include/odp/api/packet_io.h
>> @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
>>   } odp_pktio_param_t;
>>
>>   /**
>> + * Packet IO link state information
>> + */
>> +typedef struct odp_pktio_link_state_t {
>> +	uint32_t speed;		/**< Speed in Mbps: 10, 100, 1000 etc */
>> +	odp_bool_t up;		/**< 1 - link up, 0 - link down */
>> +	odp_bool_t autoneg;	/**< 1 - autoneg on, 0 - off */
>> +	odp_bool_t fd;		/**< 1 - full duplex, 0 - half duplex */
>> +	odp_bool_t stopped;	/**< 1 - pktio stopped, 0 - started */
>> +} odp_pktio_link_state_t;
>> +
>> +/**
>> + * Get packet IO link state
>> + *
>> + * @param[in] pktio	Packet IO handle
>> + * @param[out] state	Buffer to save link state
>> + *
>> + * @retval 0 on success (state info updated)
>> + * @retval <0 on failure (state info not updated)
>> + */
>> +int odp_pktio_link_state(odp_pktio_t pktio, odp_pktio_link_state_t
>> *state);
>> +
>> +/**
>>    * Open a packet IO interface
>>    *
>>    * An ODP program can open a single packet IO interface per device,
>> attempts
>> --
>> 1.9.1
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> https://lists.linaro.org/mailman/listinfo/lng-odp
Ola Liljedahl Oct. 19, 2015, 3:20 p.m. UTC | #9
On 16 October 2015 at 17:55, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> On 10/16/2015 13:24, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>> RFC 3635 helps also here to define properties of an Ethernet-like
>> interface. It's not so as crucial to follow RFC terms and definitions here
>> as it's with counters, but we can use it as a guide line. Addresses and
>> promisc mode are classification issues. Name and pktio status (stopped /
>> started) ( == ifAdminStatus) are pktio level data.
>>
>>
>> typedef struct odp_pktio_link_state_t {
>>         uint8_t  type;        // 0: Ethernet (or enum
>> ODP_PKTIO_ETHERNET), ...
>>
>
> type is not state.  I think there might be odp_pktio_info_t which is
> filled by init of each pktio.
> and some call like:
>  const odp_pktio_info_t * odp_pktio_info(pktio);
>
> where
> odp_pktio_info_t {
>
> char *name;
> uint8_t  type;        // 0: Ethernet (or enum ODP_PKTIO_ETHERNET)
>
> // what also do we need, i/o address space, number of hw queues, nodes and
> etc.
> }
>
> But it looks like Bill right pointed that for now we have separate call
> for each function.
> We already have mtu and name for example. So we need to understand if we
> are going
> to modify existence API or add new.
>
> Also I can understand that:
> 1. speed -  might be needed to set up some timeout setting.
> 2.  status link up/down and logical pktio started / stopped is needed to
> check why odp_schedule() returns timeouts instead of packets.
>
> But I have no idea why application needs type, autoneg and duplex.
>
> I think for first try we need to stay just with:
> +typedef struct odp_pktio_link_state_t {
> +       uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000 etc */
> +       odp_bool_t up;          /**< 1 - link up, 0 - link down */
> +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
>
Again, "stopped" is a poor name. Why not "running"?


> +} odp_pktio_link_state_t;
>
> I want to have up and stopped in one struct to speed that data plane calls.
>
Are these calls performance critical?



> Most likely it will be the following check
>
> ev = odp_schedule(wait_time);
> if (ev == ODP_EVENT_INVALID) {
>    odp_pktio_link_state(pktio, &link_state);
>    if (!link_state.up || link_state.stopped) {
>                 <send something to control plane,  >
>                   <or just wait for link, or pktio started>
>                   continue;
>     } else {
>         <just timeout, packet can be in any moment, ready to get them>
>         continue;
>   }
> }
>
> I.e. try to not do 2 calls and roll stack for 2 calls.
>
The example up doesn't look like it requires (or even benefits from) link
state and interface status to be obtained in the absolutely shortest time,
they are not called when we are actually processing an event.


> Does that reasonable?

No.

Interface (pktio) and physical link are different concepts which I do not
think should be mixed.



>
>
> Maxim.
>
>
>
>         uint8_t  status;      // 1: link up, 0: down, -1 not_present (==
>> ifOperStatus )
>>         uint8_t  autoneg;     // 1: autoneg on, 0: off, -1: not supported
>>         uint8_t  duplex;      // 1: full duplex, 0: half duplex
>>         uint32_t speed_mbps;  // Speed in Mbps. This leaves room for
>>                                    // speed_bps which would support odd
>> (Mbps) link speeds
>>         uint32_t mtu;         // MTU in bytes
>> } odp_pktio_link_state_t;
>>
>>
>> Better to use int (uint8_t) than bool to enable extendable status
>> definitions. E.g. ifOperStatus == not_present means that some HW component
>> is missing, rather than user have configured link down.
>>
>>
>> -Petri
>>
>>
>>
>> -----Original Message-----
>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT
>>> Maxim Uvarov
>>> Sent: Tuesday, October 13, 2015 3:39 PM
>>> To: lng-odp@lists.linaro.org
>>> Subject: [lng-odp] [API-NEXT PATCHv2] api: pktio link
>>>
>>> Define API to get pktio link state: seed, autoneg, link up/down,
>>> duplex, started/stopped state.
>>>
>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>>> ---
>>>   v2: - use simple struct to return pktio link state;
>>>       - odp will not modify link, only ready it's state;
>>>
>>>
>>>   include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
>>> index d8e69ed..6c77e8d 100644
>>> --- a/include/odp/api/packet_io.h
>>> +++ b/include/odp/api/packet_io.h
>>> @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
>>>   } odp_pktio_param_t;
>>>
>>>   /**
>>> + * Packet IO link state information
>>> + */
>>> +typedef struct odp_pktio_link_state_t {
>>> +       uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000 etc */
>>> +       odp_bool_t up;          /**< 1 - link up, 0 - link down */
>>> +       odp_bool_t autoneg;     /**< 1 - autoneg on, 0 - off */
>>> +       odp_bool_t fd;          /**< 1 - full duplex, 0 - half duplex */
>>> +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
>>> +} odp_pktio_link_state_t;
>>> +
>>> +/**
>>> + * Get packet IO link state
>>> + *
>>> + * @param[in] pktio    Packet IO handle
>>> + * @param[out] state   Buffer to save link state
>>> + *
>>> + * @retval 0 on success (state info updated)
>>> + * @retval <0 on failure (state info not updated)
>>> + */
>>> +int odp_pktio_link_state(odp_pktio_t pktio, odp_pktio_link_state_t
>>> *state);
>>> +
>>> +/**
>>>    * Open a packet IO interface
>>>    *
>>>    * An ODP program can open a single packet IO interface per device,
>>> attempts
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> https://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
Maxim Uvarov Oct. 19, 2015, 3:33 p.m. UTC | #10
On 10/19/2015 18:20, Ola Liljedahl wrote:
> On 16 October 2015 at 17:55, Maxim Uvarov <maxim.uvarov@linaro.org 
> <mailto:maxim.uvarov@linaro.org>> wrote:
>
>     On 10/16/2015 13:24, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>         RFC 3635 helps also here to define properties of an
>         Ethernet-like interface. It's not so as crucial to follow RFC
>         terms and definitions here as it's with counters, but we can
>         use it as a guide line. Addresses and promisc mode are
>         classification issues. Name and pktio status (stopped /
>         started) ( == ifAdminStatus) are pktio level data.
>
>
>         typedef struct odp_pktio_link_state_t {
>                 uint8_t  type;        // 0: Ethernet (or enum
>         ODP_PKTIO_ETHERNET), ...
>
>
>     type is not state.  I think there might be odp_pktio_info_t which
>     is filled by init of each pktio.
>     and some call like:
>      const odp_pktio_info_t * odp_pktio_info(pktio);
>
>     where
>     odp_pktio_info_t {
>
>     char *name;
>     uint8_t  type;        // 0: Ethernet (or enum ODP_PKTIO_ETHERNET)
>
>     // what also do we need, i/o address space, number of hw queues,
>     nodes and etc.
>     }
>
>     But it looks like Bill right pointed that for now we have separate
>     call for each function.
>     We already have mtu and name for example. So we need to understand
>     if we are going
>     to modify existence API or add new.
>
>     Also I can understand that:
>     1. speed -  might be needed to set up some timeout setting.
>     2.  status link up/down and logical pktio started / stopped is
>     needed to check why odp_schedule() returns timeouts instead of
>     packets.
>
>     But I have no idea why application needs type, autoneg and duplex.
>
>     I think for first try we need to stay just with:
>     +typedef struct odp_pktio_link_state_t {
>     +       uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000
>     etc */
>     +       odp_bool_t up;          /**< 1 - link up, 0 - link down */
>     +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
>
> Again, "stopped" is a poor name. Why not "running"?

Just serious why "stopped" is poor name and "running" is good name? In 
future we might
have several states for pktio, something like paused. More likely it 
will be better to have just
state which is some state enum.

>     +} odp_pktio_link_state_t;
>
>     I want to have up and stopped in one struct to speed that data
>     plane calls.
>
> Are these calls performance critical?
>
>     Most likely it will be the following check
>
>     ev = odp_schedule(wait_time);
>     if (ev == ODP_EVENT_INVALID) {
>        odp_pktio_link_state(pktio, &link_state);
>        if (!link_state.up || link_state.stopped) {
>                     <send something to control plane,  >
>                       <or just wait for link, or pktio started>
>                       continue;
>         } else {
>             <just timeout, packet can be in any moment, ready to get them>
>             continue;
>       }
>     }
>
>     I.e. try to not do 2 calls and roll stack for 2 calls.
>
> The example up doesn't look like it requires (or even benefits from) 
> link state and interface status to be obtained in the absolutely 
> shortest time, they are not called when we are actually processing an 
> event.
>
>
>     Does that reasonable?
>
> No.
>
> Interface (pktio) and physical link are different concepts which I do 
> not think should be mixed.
>

Ok, if there is no reason, then I will send patch only with  up/down and 
speed.

Maxim.

>
>
>     Maxim.
>
>
>
>               uint8_t  status;      // 1: link up, 0: down, -1
>         not_present (== ifOperStatus )
>                 uint8_t  autoneg;     // 1: autoneg on, 0: off, -1:
>         not supported
>                 uint8_t  duplex;      // 1: full duplex, 0: half duplex
>                 uint32_t speed_mbps;  // Speed in Mbps. This leaves
>         room for
>                                            // speed_bps which would
>         support odd (Mbps) link speeds
>                 uint32_t mtu;         // MTU in bytes
>         } odp_pktio_link_state_t;
>
>
>         Better to use int (uint8_t) than bool to enable extendable
>         status definitions. E.g. ifOperStatus == not_present means
>         that some HW component is missing, rather than user have
>         configured link down.
>
>
>         -Petri
>
>
>
>             -----Original Message-----
>             From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org
>             <mailto:lng-odp-bounces@lists.linaro.org>] On Behalf Of EXT
>             Maxim Uvarov
>             Sent: Tuesday, October 13, 2015 3:39 PM
>             To: lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>             Subject: [lng-odp] [API-NEXT PATCHv2] api: pktio link
>
>             Define API to get pktio link state: seed, autoneg, link
>             up/down,
>             duplex, started/stopped state.
>
>             Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>>
>             ---
>               v2: - use simple struct to return pktio link state;
>                   - odp will not modify link, only ready it's state;
>
>
>               include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
>               1 file changed, 22 insertions(+)
>
>             diff --git a/include/odp/api/packet_io.h
>             b/include/odp/api/packet_io.h
>             index d8e69ed..6c77e8d 100644
>             --- a/include/odp/api/packet_io.h
>             +++ b/include/odp/api/packet_io.h
>             @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
>               } odp_pktio_param_t;
>
>               /**
>             + * Packet IO link state information
>             + */
>             +typedef struct odp_pktio_link_state_t {
>             +       uint32_t speed;         /**< Speed in Mbps: 10,
>             100, 1000 etc */
>             +       odp_bool_t up;          /**< 1 - link up, 0 - link
>             down */
>             +       odp_bool_t autoneg;     /**< 1 - autoneg on, 0 -
>             off */
>             +       odp_bool_t fd;          /**< 1 - full duplex, 0 -
>             half duplex */
>             +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0
>             - started */
>             +} odp_pktio_link_state_t;
>             +
>             +/**
>             + * Get packet IO link state
>             + *
>             + * @param[in] pktio    Packet IO handle
>             + * @param[out] state   Buffer to save link state
>             + *
>             + * @retval 0 on success (state info updated)
>             + * @retval <0 on failure (state info not updated)
>             + */
>             +int odp_pktio_link_state(odp_pktio_t pktio,
>             odp_pktio_link_state_t
>             *state);
>             +
>             +/**
>                * Open a packet IO interface
>                *
>                * An ODP program can open a single packet IO interface
>             per device,
>             attempts
>             --
>             1.9.1
>
>             _______________________________________________
>             lng-odp mailing list
>             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>             https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>     _______________________________________________
>     lng-odp mailing list
>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>     https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Ola Liljedahl Oct. 19, 2015, 3:39 p.m. UTC | #11
On 19 October 2015 at 17:33, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> On 10/19/2015 18:20, Ola Liljedahl wrote:
>
>> On 16 October 2015 at 17:55, Maxim Uvarov <maxim.uvarov@linaro.org
>> <mailto:maxim.uvarov@linaro.org>> wrote:
>>
>>     On 10/16/2015 13:24, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>
>>         RFC 3635 helps also here to define properties of an
>>         Ethernet-like interface. It's not so as crucial to follow RFC
>>         terms and definitions here as it's with counters, but we can
>>         use it as a guide line. Addresses and promisc mode are
>>         classification issues. Name and pktio status (stopped /
>>         started) ( == ifAdminStatus) are pktio level data.
>>
>>
>>         typedef struct odp_pktio_link_state_t {
>>                 uint8_t  type;        // 0: Ethernet (or enum
>>         ODP_PKTIO_ETHERNET), ...
>>
>>
>>     type is not state.  I think there might be odp_pktio_info_t which
>>     is filled by init of each pktio.
>>     and some call like:
>>      const odp_pktio_info_t * odp_pktio_info(pktio);
>>
>>     where
>>     odp_pktio_info_t {
>>
>>     char *name;
>>     uint8_t  type;        // 0: Ethernet (or enum ODP_PKTIO_ETHERNET)
>>
>>     // what also do we need, i/o address space, number of hw queues,
>>     nodes and etc.
>>     }
>>
>>     But it looks like Bill right pointed that for now we have separate
>>     call for each function.
>>     We already have mtu and name for example. So we need to understand
>>     if we are going
>>     to modify existence API or add new.
>>
>>     Also I can understand that:
>>     1. speed -  might be needed to set up some timeout setting.
>>     2.  status link up/down and logical pktio started / stopped is
>>     needed to check why odp_schedule() returns timeouts instead of
>>     packets.
>>
>>     But I have no idea why application needs type, autoneg and duplex.
>>
>>     I think for first try we need to stay just with:
>>     +typedef struct odp_pktio_link_state_t {
>>     +       uint32_t speed;         /**< Speed in Mbps: 10, 100, 1000
>>     etc */
>>     +       odp_bool_t up;          /**< 1 - link up, 0 - link down */
>>     +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0 - started */
>>
>> Again, "stopped" is a poor name. Why not "running"?
>>
>
> Just serious why "stopped" is poor name and "running" is good name?

Because "stopped" (or "started") relates to the action (e.g. stop or start)
you requested in order to change the state.
We don't call the link state variable "upped" or "downed" etc.



> In future we might
> have several states for pktio, something like paused. More likely it will
> be better to have just
> state which is some state enum.
>
>     +} odp_pktio_link_state_t;
>>
> Yes better.


>
>>     I want to have up and stopped in one struct to speed that data
>>     plane calls.
>>
>> Are these calls performance critical?
>>
>>     Most likely it will be the following check
>>
>>     ev = odp_schedule(wait_time);
>>     if (ev == ODP_EVENT_INVALID) {
>>        odp_pktio_link_state(pktio, &link_state);
>>        if (!link_state.up || link_state.stopped) {
>>                     <send something to control plane,  >
>>                       <or just wait for link, or pktio started>
>>                       continue;
>>         } else {
>>             <just timeout, packet can be in any moment, ready to get them>
>>             continue;
>>       }
>>     }
>>
>>     I.e. try to not do 2 calls and roll stack for 2 calls.
>>
>> The example up doesn't look like it requires (or even benefits from) link
>> state and interface status to be obtained in the absolutely shortest time,
>> they are not called when we are actually processing an event.
>>
>>
>>     Does that reasonable?
>>
>> No.
>>
>> Interface (pktio) and physical link are different concepts which I do not
>> think should be mixed.
>>
>>
> Ok, if there is no reason, then I will send patch only with  up/down and
> speed.
>
Good.


>
> Maxim.
>
>
>>
>>     Maxim.
>>
>>
>>
>>               uint8_t  status;      // 1: link up, 0: down, -1
>>         not_present (== ifOperStatus )
>>                 uint8_t  autoneg;     // 1: autoneg on, 0: off, -1:
>>         not supported
>>                 uint8_t  duplex;      // 1: full duplex, 0: half duplex
>>                 uint32_t speed_mbps;  // Speed in Mbps. This leaves
>>         room for
>>                                            // speed_bps which would
>>         support odd (Mbps) link speeds
>>                 uint32_t mtu;         // MTU in bytes
>>         } odp_pktio_link_state_t;
>>
>>
>>         Better to use int (uint8_t) than bool to enable extendable
>>         status definitions. E.g. ifOperStatus == not_present means
>>         that some HW component is missing, rather than user have
>>         configured link down.
>>
>>
>>         -Petri
>>
>>
>>
>>             -----Original Message-----
>>             From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org
>>             <mailto:lng-odp-bounces@lists.linaro.org>] On Behalf Of EXT
>>             Maxim Uvarov
>>             Sent: Tuesday, October 13, 2015 3:39 PM
>>             To: lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org
>> >
>>             Subject: [lng-odp] [API-NEXT PATCHv2] api: pktio link
>>
>>             Define API to get pktio link state: seed, autoneg, link
>>             up/down,
>>             duplex, started/stopped state.
>>
>>             Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>>             <mailto:maxim.uvarov@linaro.org>>
>>             ---
>>               v2: - use simple struct to return pktio link state;
>>                   - odp will not modify link, only ready it's state;
>>
>>
>>               include/odp/api/packet_io.h | 22 ++++++++++++++++++++++
>>               1 file changed, 22 insertions(+)
>>
>>             diff --git a/include/odp/api/packet_io.h
>>             b/include/odp/api/packet_io.h
>>             index d8e69ed..6c77e8d 100644
>>             --- a/include/odp/api/packet_io.h
>>             +++ b/include/odp/api/packet_io.h
>>             @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
>>               } odp_pktio_param_t;
>>
>>               /**
>>             + * Packet IO link state information
>>             + */
>>             +typedef struct odp_pktio_link_state_t {
>>             +       uint32_t speed;         /**< Speed in Mbps: 10,
>>             100, 1000 etc */
>>             +       odp_bool_t up;          /**< 1 - link up, 0 - link
>>             down */
>>             +       odp_bool_t autoneg;     /**< 1 - autoneg on, 0 -
>>             off */
>>             +       odp_bool_t fd;          /**< 1 - full duplex, 0 -
>>             half duplex */
>>             +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0
>>             - started */
>>             +} odp_pktio_link_state_t;
>>             +
>>             +/**
>>             + * Get packet IO link state
>>             + *
>>             + * @param[in] pktio    Packet IO handle
>>             + * @param[out] state   Buffer to save link state
>>             + *
>>             + * @retval 0 on success (state info updated)
>>             + * @retval <0 on failure (state info not updated)
>>             + */
>>             +int odp_pktio_link_state(odp_pktio_t pktio,
>>             odp_pktio_link_state_t
>>             *state);
>>             +
>>             +/**
>>                * Open a packet IO interface
>>                *
>>                * An ODP program can open a single packet IO interface
>>             per device,
>>             attempts
>>             --
>>             1.9.1
>>
>>             _______________________________________________
>>             lng-odp mailing list
>>             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>             https://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>     _______________________________________________
>>     lng-odp mailing list
>>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>     https://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>
>
Maxim Uvarov Oct. 19, 2015, 4:14 p.m. UTC | #12
On 10/19/2015 18:39, Ola Liljedahl wrote:
> On 19 October 2015 at 17:33, Maxim Uvarov <maxim.uvarov@linaro.org 
> <mailto:maxim.uvarov@linaro.org>> wrote:
>
>     On 10/19/2015 18:20, Ola Liljedahl wrote:
>
>         On 16 October 2015 at 17:55, Maxim Uvarov
>         <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
>         <mailto:maxim.uvarov@linaro.org
>         <mailto:maxim.uvarov@linaro.org>>> wrote:
>
>             On 10/16/2015 13:24, Savolainen, Petri (Nokia - FI/Espoo)
>         wrote:
>
>                 RFC 3635 helps also here to define properties of an
>                 Ethernet-like interface. It's not so as crucial to
>         follow RFC
>                 terms and definitions here as it's with counters, but
>         we can
>                 use it as a guide line. Addresses and promisc mode are
>                 classification issues. Name and pktio status (stopped /
>                 started) ( == ifAdminStatus) are pktio level data.
>
>
>                 typedef struct odp_pktio_link_state_t {
>                         uint8_t  type;        // 0: Ethernet (or enum
>                 ODP_PKTIO_ETHERNET), ...
>
>
>             type is not state.  I think there might be
>         odp_pktio_info_t which
>             is filled by init of each pktio.
>             and some call like:
>              const odp_pktio_info_t * odp_pktio_info(pktio);
>
>             where
>             odp_pktio_info_t {
>
>             char *name;
>             uint8_t  type;        // 0: Ethernet (or enum
>         ODP_PKTIO_ETHERNET)
>
>             // what also do we need, i/o address space, number of hw
>         queues,
>             nodes and etc.
>             }
>
>             But it looks like Bill right pointed that for now we have
>         separate
>             call for each function.
>             We already have mtu and name for example. So we need to
>         understand
>             if we are going
>             to modify existence API or add new.
>
>             Also I can understand that:
>             1. speed -  might be needed to set up some timeout setting.
>             2.  status link up/down and logical pktio started / stopped is
>             needed to check why odp_schedule() returns timeouts instead of
>             packets.
>
>             But I have no idea why application needs type, autoneg and
>         duplex.
>
>             I think for first try we need to stay just with:
>             +typedef struct odp_pktio_link_state_t {
>             +       uint32_t speed;         /**< Speed in Mbps: 10,
>         100, 1000
>             etc */
>             +       odp_bool_t up;          /**< 1 - link up, 0 - link
>         down */
>             +       odp_bool_t stopped;     /**< 1 - pktio stopped, 0
>         - started */
>
>         Again, "stopped" is a poor name. Why not "running"?
>
>
>     Just serious why "stopped" is poor name and "running" is good name?
>
> Because "stopped" (or "started") relates to the action (e.g. stop or 
> start) you requested in order to change the state.
> We don't call the link state variable "upped" or "downed" etc.
>

got it, thanks.
>
>     In future we might
>     have several states for pktio, something like paused. More likely
>     it will be better to have just
>     state which is some state enum.
>
>             +} odp_pktio_link_state_t;
>
> Yes better.
>
>
>             I want to have up and stopped in one struct to speed that data
>             plane calls.
>
>         Are these calls performance critical?
>
>             Most likely it will be the following check
>
>             ev = odp_schedule(wait_time);
>             if (ev == ODP_EVENT_INVALID) {
>                odp_pktio_link_state(pktio, &link_state);
>                if (!link_state.up || link_state.stopped) {
>                             <send something to control plane,  >
>                               <or just wait for link, or pktio started>
>                               continue;
>                 } else {
>                     <just timeout, packet can be in any moment, ready
>         to get them>
>                     continue;
>               }
>             }
>
>             I.e. try to not do 2 calls and roll stack for 2 calls.
>
>         The example up doesn't look like it requires (or even benefits
>         from) link state and interface status to be obtained in the
>         absolutely shortest time, they are not called when we are
>         actually processing an event.
>
>
>             Does that reasonable?
>
>         No.
>
>         Interface (pktio) and physical link are different concepts
>         which I do not think should be mixed.
>
>
>     Ok, if there is no reason, then I will send patch only with 
>     up/down and speed.
>
> Good.
>
>
>     Maxim.
>
>
>
>             Maxim.
>
>
>
>                       uint8_t  status;      // 1: link up, 0: down, -1
>                 not_present (== ifOperStatus )
>                         uint8_t  autoneg;     // 1: autoneg on, 0:
>         off, -1:
>                 not supported
>                         uint8_t  duplex;      // 1: full duplex, 0:
>         half duplex
>                         uint32_t speed_mbps;  // Speed in Mbps. This
>         leaves
>                 room for
>                                                    // speed_bps which
>         would
>                 support odd (Mbps) link speeds
>                         uint32_t mtu;         // MTU in bytes
>                 } odp_pktio_link_state_t;
>
>
>                 Better to use int (uint8_t) than bool to enable extendable
>                 status definitions. E.g. ifOperStatus == not_present means
>                 that some HW component is missing, rather than user have
>                 configured link down.
>
>
>                 -Petri
>
>
>
>                     -----Original Message-----
>                     From: lng-odp
>         [mailto:lng-odp-bounces@lists.linaro.org
>         <mailto:lng-odp-bounces@lists.linaro.org>
>                     <mailto:lng-odp-bounces@lists.linaro.org
>         <mailto:lng-odp-bounces@lists.linaro.org>>] On Behalf Of EXT
>                     Maxim Uvarov
>                     Sent: Tuesday, October 13, 2015 3:39 PM
>                     To: lng-odp@lists.linaro.org
>         <mailto:lng-odp@lists.linaro.org>
>         <mailto:lng-odp@lists.linaro.org
>         <mailto:lng-odp@lists.linaro.org>>
>                     Subject: [lng-odp] [API-NEXT PATCHv2] api: pktio link
>
>                     Define API to get pktio link state: seed, autoneg,
>         link
>                     up/down,
>                     duplex, started/stopped state.
>
>                     Signed-off-by: Maxim Uvarov
>         <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
>                     <mailto:maxim.uvarov@linaro.org
>         <mailto:maxim.uvarov@linaro.org>>>
>                     ---
>                       v2: - use simple struct to return pktio link state;
>                           - odp will not modify link, only ready it's
>         state;
>
>
>                       include/odp/api/packet_io.h | 22
>         ++++++++++++++++++++++
>                       1 file changed, 22 insertions(+)
>
>                     diff --git a/include/odp/api/packet_io.h
>                     b/include/odp/api/packet_io.h
>                     index d8e69ed..6c77e8d 100644
>                     --- a/include/odp/api/packet_io.h
>                     +++ b/include/odp/api/packet_io.h
>                     @@ -96,6 +96,28 @@ typedef struct odp_pktio_param_t {
>                       } odp_pktio_param_t;
>
>                       /**
>                     + * Packet IO link state information
>                     + */
>                     +typedef struct odp_pktio_link_state_t {
>                     +       uint32_t speed;         /**< Speed in
>         Mbps: 10,
>                     100, 1000 etc */
>                     +       odp_bool_t up;          /**< 1 - link up,
>         0 - link
>                     down */
>                     +       odp_bool_t autoneg;     /**< 1 - autoneg
>         on, 0 -
>                     off */
>                     +       odp_bool_t fd;          /**< 1 - full
>         duplex, 0 -
>                     half duplex */
>                     +       odp_bool_t stopped;     /**< 1 - pktio
>         stopped, 0
>                     - started */
>                     +} odp_pktio_link_state_t;
>                     +
>                     +/**
>                     + * Get packet IO link state
>                     + *
>                     + * @param[in] pktio    Packet IO handle
>                     + * @param[out] state   Buffer to save link state
>                     + *
>                     + * @retval 0 on success (state info updated)
>                     + * @retval <0 on failure (state info not updated)
>                     + */
>                     +int odp_pktio_link_state(odp_pktio_t pktio,
>                     odp_pktio_link_state_t
>                     *state);
>                     +
>                     +/**
>                        * Open a packet IO interface
>                        *
>                        * An ODP program can open a single packet IO
>         interface
>                     per device,
>                     attempts
>                     --
>                     1.9.1
>
>         _______________________________________________
>                     lng-odp mailing list
>         lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>         <mailto:lng-odp@lists.linaro.org
>         <mailto:lng-odp@lists.linaro.org>>
>         https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>             _______________________________________________
>             lng-odp mailing list
>         lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>         <mailto:lng-odp@lists.linaro.org
>         <mailto:lng-odp@lists.linaro.org>>
>         https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
>
diff mbox

Patch

diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
index d8e69ed..6c77e8d 100644
--- a/include/odp/api/packet_io.h
+++ b/include/odp/api/packet_io.h
@@ -96,6 +96,28 @@  typedef struct odp_pktio_param_t {
 } odp_pktio_param_t;
 
 /**
+ * Packet IO link state information
+ */
+typedef struct odp_pktio_link_state_t {
+	uint32_t speed;		/**< Speed in Mbps: 10, 100, 1000 etc */
+	odp_bool_t up;		/**< 1 - link up, 0 - link down */
+	odp_bool_t autoneg;	/**< 1 - autoneg on, 0 - off */
+	odp_bool_t fd;		/**< 1 - full duplex, 0 - half duplex */
+	odp_bool_t stopped;	/**< 1 - pktio stopped, 0 - started */
+} odp_pktio_link_state_t;
+
+/**
+ * Get packet IO link state
+ *
+ * @param[in] pktio	Packet IO handle
+ * @param[out] state	Buffer to save link state
+ *
+ * @retval 0 on success (state info updated)
+ * @retval <0 on failure (state info not updated)
+ */
+int odp_pktio_link_state(odp_pktio_t pktio, odp_pktio_link_state_t *state);
+
+/**
  * Open a packet IO interface
  *
  * An ODP program can open a single packet IO interface per device, attempts