doc: announce changes to ethdev rxconf structure

Message ID 1596452291-25535-1-git-send-email-viacheslavo@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series doc: announce changes to ethdev rxconf structure |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Slava Ovsiienko Aug. 3, 2020, 10:58 a.m. UTC
  The DPDK datapath in the transmit direction is very flexible.
The applications can build multisegment packets and manages
almost all data aspects - the memory pools where segments
are allocated from, the segment lengths, the memory attributes
like external, registered, etc.

In the receiving direction, the datapath is much less flexible,
the applications can only specify the memory pool to configure
the receiving queue and nothing more. In order to extend the
receiving datapath capabilities it is proposed to add the new
fields into rte_eth_rxconf structure:

struct rte_eth_rxconf {
    ...
    uint16_t rx_split_num; /* number of segments to split */
    uint16_t *rx_split_len; /* array of segment lengthes */
    struct rte_mempool **mp; /* array of segment memory pools */
    ...
};

The non-zero value of rx_split_num field configures the receiving
queue to split ingress packets into multiple segments to the mbufs
allocated from various memory pools according to the specified
lengths. The zero value of rx_split_num field provides the
backward compatibility and queue should be configured in a regular
way (with single/multiple mbufs of the same data buffer length
allocated from the single memory pool).

The new approach would allow splitting the ingress packets into
multiple parts pushed to the memory with different attributes.
For example, the packet headers can be pushed to the embedded data
buffers within mbufs and the application data into the external
buffers attached to mbufs allocated from the different memory
pools. The memory attributes for the split parts may differ
either - for example the application data may be pushed into
the external memory located on the dedicated physical device,
say GPU or NVMe. This would improve the DPDK receiving datapath
flexibility preserving compatibility with existing API.

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 +++++
 1 file changed, 5 insertions(+)
  

Comments

Jerin Jacob Aug. 3, 2020, 11:56 a.m. UTC | #1
On Mon, Aug 3, 2020 at 4:28 PM Viacheslav Ovsiienko
<viacheslavo@mellanox.com> wrote:
>
> The DPDK datapath in the transmit direction is very flexible.
> The applications can build multisegment packets and manages
> almost all data aspects - the memory pools where segments
> are allocated from, the segment lengths, the memory attributes
> like external, registered, etc.
>
> In the receiving direction, the datapath is much less flexible,
> the applications can only specify the memory pool to configure
> the receiving queue and nothing more. In order to extend the
> receiving datapath capabilities it is proposed to add the new
> fields into rte_eth_rxconf structure:
>
> struct rte_eth_rxconf {
>     ...
>     uint16_t rx_split_num; /* number of segments to split */
>     uint16_t *rx_split_len; /* array of segment lengthes */
>     struct rte_mempool **mp; /* array of segment memory pools */

The pool has the packet length it's been configured for.
So I think, rx_split_len can be removed.

This feature also available in Marvell HW. So it not specific to one vendor.
Maybe we could just the use case mention the use case in the depreciation notice
and the tentative change in rte_eth_rxconf and exact details can be worked
out at the time of implementation.

With the above change:
Acked-by: Jerin Jacob <jerinj@marvell.com>


>     ...
> };
>
> The non-zero value of rx_split_num field configures the receiving
> queue to split ingress packets into multiple segments to the mbufs
> allocated from various memory pools according to the specified
> lengths. The zero value of rx_split_num field provides the
> backward compatibility and queue should be configured in a regular
> way (with single/multiple mbufs of the same data buffer length
> allocated from the single memory pool).
>
> The new approach would allow splitting the ingress packets into
> multiple parts pushed to the memory with different attributes.
> For example, the packet headers can be pushed to the embedded data
> buffers within mbufs and the application data into the external
> buffers attached to mbufs allocated from the different memory
> pools. The memory attributes for the split parts may differ
> either - for example the application data may be pushed into
> the external memory located on the dedicated physical device,
> say GPU or NVMe. This would improve the DPDK receiving datapath
> flexibility preserving compatibility with existing API.
>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index ea4cfa7..cd700ae 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -99,6 +99,11 @@ Deprecation Notices
>    In 19.11 PMDs will still update the field even when the offload is not
>    enabled.
>
> +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the receiving
> +  queues to split ingress packets into multiple segments according to the
> +  specified lengths into the buffers allocated from the specified
> +  memory pools. The backward compatibility to existing API is preserved.
> +
>  * ethdev: ``rx_descriptor_done`` dev_ops and ``rte_eth_rx_descriptor_done``
>    will be deprecated in 20.11 and will be removed in 21.11.
>    Existing ``rte_eth_rx_descriptor_status`` and ``rte_eth_tx_descriptor_status``
> --
> 1.8.3.1
>
  
Slava Ovsiienko Aug. 3, 2020, 1:06 p.m. UTC | #2
Hi, Jerin,

Thanks for the comment,  please, see below.

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Monday, August 3, 2020 14:57
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@mellanox.com>;
> Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Stephen
> Hemminger <stephen@networkplumber.org>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Olivier Matz <olivier.matz@6wind.com>;
> David Marchand <david.marchand@redhat.com>
> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> 
> On Mon, Aug 3, 2020 at 4:28 PM Viacheslav Ovsiienko
> <viacheslavo@mellanox.com> wrote:
> >
> > The DPDK datapath in the transmit direction is very flexible.
> > The applications can build multisegment packets and manages almost all
> > data aspects - the memory pools where segments are allocated from, the
> > segment lengths, the memory attributes like external, registered, etc.
> >
> > In the receiving direction, the datapath is much less flexible, the
> > applications can only specify the memory pool to configure the
> > receiving queue and nothing more. In order to extend the receiving
> > datapath capabilities it is proposed to add the new fields into
> > rte_eth_rxconf structure:
> >
> > struct rte_eth_rxconf {
> >     ...
> >     uint16_t rx_split_num; /* number of segments to split */
> >     uint16_t *rx_split_len; /* array of segment lengthes */
> >     struct rte_mempool **mp; /* array of segment memory pools */
> 
> The pool has the packet length it's been configured for.
> So I think, rx_split_len can be removed.

Yes, it is one of the supposed options - if pointer to array of segment lengths
is NULL , the queue_setup() could use the lengths from the pool's properties.
But we are talking about packet split, in general, it should not depend
on pool properties. What if application provides the single pool
and just wants to have the tunnel header in the first dedicated mbuf?

> 
> This feature also available in Marvell HW. So it not specific to one vendor.
> Maybe we could just the use case mention the use case in the depreciation
> notice and the tentative change in rte_eth_rxconf and exact details can be
> worked out at the time of implementation.
> 
So, if I understand correctly, the struct changes in the commit message
should be marked as just possible implementation?

With best regards,
Slava

> With the above change:
> Acked-by: Jerin Jacob <jerinj@marvell.com>
> 
> 
> >     ...
> > };
> >
> > The non-zero value of rx_split_num field configures the receiving
> > queue to split ingress packets into multiple segments to the mbufs
> > allocated from various memory pools according to the specified
> > lengths. The zero value of rx_split_num field provides the backward
> > compatibility and queue should be configured in a regular way (with
> > single/multiple mbufs of the same data buffer length allocated from
> > the single memory pool).
> >
> > The new approach would allow splitting the ingress packets into
> > multiple parts pushed to the memory with different attributes.
> > For example, the packet headers can be pushed to the embedded data
> > buffers within mbufs and the application data into the external
> > buffers attached to mbufs allocated from the different memory pools.
> > The memory attributes for the split parts may differ either - for
> > example the application data may be pushed into the external memory
> > located on the dedicated physical device, say GPU or NVMe. This would
> > improve the DPDK receiving datapath flexibility preserving
> > compatibility with existing API.
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
> >  doc/guides/rel_notes/deprecation.rst | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index ea4cfa7..cd700ae 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -99,6 +99,11 @@ Deprecation Notices
> >    In 19.11 PMDs will still update the field even when the offload is not
> >    enabled.
> >
> > +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the
> > +receiving
> > +  queues to split ingress packets into multiple segments according to
> > +the
> > +  specified lengths into the buffers allocated from the specified
> > +  memory pools. The backward compatibility to existing API is preserved.
> > +
> >  * ethdev: ``rx_descriptor_done`` dev_ops and
> ``rte_eth_rx_descriptor_done``
> >    will be deprecated in 20.11 and will be removed in 21.11.
> >    Existing ``rte_eth_rx_descriptor_status`` and
> > ``rte_eth_tx_descriptor_status``
> > --
> > 1.8.3.1
> >
  
Andrew Rybchenko Aug. 3, 2020, 2:31 p.m. UTC | #3
On 8/3/20 1:58 PM, Viacheslav Ovsiienko wrote:
> The DPDK datapath in the transmit direction is very flexible.
> The applications can build multisegment packets and manages
> almost all data aspects - the memory pools where segments
> are allocated from, the segment lengths, the memory attributes
> like external, registered, etc.
>
> In the receiving direction, the datapath is much less flexible,
> the applications can only specify the memory pool to configure
> the receiving queue and nothing more. In order to extend the
> receiving datapath capabilities it is proposed to add the new
> fields into rte_eth_rxconf structure:
>
> struct rte_eth_rxconf {
>     ...
>     uint16_t rx_split_num; /* number of segments to split */
>     uint16_t *rx_split_len; /* array of segment lengthes */
>     struct rte_mempool **mp; /* array of segment memory pools */
>     ...
> };
>
> The non-zero value of rx_split_num field configures the receiving
> queue to split ingress packets into multiple segments to the mbufs
> allocated from various memory pools according to the specified
> lengths. The zero value of rx_split_num field provides the
> backward compatibility and queue should be configured in a regular
> way (with single/multiple mbufs of the same data buffer length
> allocated from the single memory pool).

From the above description it is not 100% clear how it will
coexist with:
 - existing mb_pool argument of the rte_eth_rx_queue_setup()
 - DEV_RX_OFFLOAD_SCATTER
 - DEV_RX_OFFLOAD_HEADER_SPLIT
How will application know that the feature is supported? Limitations?
Is it always split by specified/fixed length?
What happens if header length is actually different?

> The new approach would allow splitting the ingress packets into
> multiple parts pushed to the memory with different attributes.
> For example, the packet headers can be pushed to the embedded data
> buffers within mbufs and the application data into the external
> buffers attached to mbufs allocated from the different memory
> pools. The memory attributes for the split parts may differ
> either - for example the application data may be pushed into
> the external memory located on the dedicated physical device,
> say GPU or NVMe. This would improve the DPDK receiving datapath
> flexibility preserving compatibility with existing API.
>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index ea4cfa7..cd700ae 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -99,6 +99,11 @@ Deprecation Notices
>    In 19.11 PMDs will still update the field even when the offload is not
>    enabled.
>  
> +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the receiving
> +  queues to split ingress packets into multiple segments according to the
> +  specified lengths into the buffers allocated from the specified
> +  memory pools. The backward compatibility to existing API is preserved.
> +
>  * ethdev: ``rx_descriptor_done`` dev_ops and ``rte_eth_rx_descriptor_done``
>    will be deprecated in 20.11 and will be removed in 21.11.
>    Existing ``rte_eth_rx_descriptor_status`` and ``rte_eth_tx_descriptor_status``
  
Jerin Jacob Aug. 4, 2020, 1:32 p.m. UTC | #4
On Mon, Aug 3, 2020 at 6:36 PM Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
>
> Hi, Jerin,
>
> Thanks for the comment,  please, see below.
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Monday, August 3, 2020 14:57
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@mellanox.com>;
> > Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Stephen
> > Hemminger <stephen@networkplumber.org>; Andrew Rybchenko
> > <arybchenko@solarflare.com>; Ajit Khaparde
> > <ajit.khaparde@broadcom.com>; Maxime Coquelin
> > <maxime.coquelin@redhat.com>; Olivier Matz <olivier.matz@6wind.com>;
> > David Marchand <david.marchand@redhat.com>
> > Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> >
> > On Mon, Aug 3, 2020 at 4:28 PM Viacheslav Ovsiienko
> > <viacheslavo@mellanox.com> wrote:
> > >
> > > The DPDK datapath in the transmit direction is very flexible.
> > > The applications can build multisegment packets and manages almost all
> > > data aspects - the memory pools where segments are allocated from, the
> > > segment lengths, the memory attributes like external, registered, etc.
> > >
> > > In the receiving direction, the datapath is much less flexible, the
> > > applications can only specify the memory pool to configure the
> > > receiving queue and nothing more. In order to extend the receiving
> > > datapath capabilities it is proposed to add the new fields into
> > > rte_eth_rxconf structure:
> > >
> > > struct rte_eth_rxconf {
> > >     ...
> > >     uint16_t rx_split_num; /* number of segments to split */
> > >     uint16_t *rx_split_len; /* array of segment lengthes */
> > >     struct rte_mempool **mp; /* array of segment memory pools */
> >
> > The pool has the packet length it's been configured for.
> > So I think, rx_split_len can be removed.
>
> Yes, it is one of the supposed options - if pointer to array of segment lengths
> is NULL , the queue_setup() could use the lengths from the pool's properties.
> But we are talking about packet split, in general, it should not depend
> on pool properties. What if application provides the single pool
> and just wants to have the tunnel header in the first dedicated mbuf?
>
> >
> > This feature also available in Marvell HW. So it not specific to one vendor.
> > Maybe we could just the use case mention the use case in the depreciation
> > notice and the tentative change in rte_eth_rxconf and exact details can be
> > worked out at the time of implementation.
> >
> So, if I understand correctly, the struct changes in the commit message
> should be marked as just possible implementation?

Yes.

We may need to have a detailed discussion on the correct abstraction for various
HW is available with this feature.

On Marvell HW, We can configure TWO pools for given eth Rx queue.
One pool can be configured as a small packet pool and other one as
large packet pool.
And there is a threshold value to decide the pool between small and large.
For example:
- The small pool is configured 2k
- The large pool is configured with 10k
- And if the threshold value is configured as 2k.
Any packet size <=2K will land in small pool and others in a large pool.
The use case, we are targeting is to save the memory space for jumbo frames.

If you can share the MLX HW working model, Then we can find the
correct abstraction.

>
> With best regards,
> Slava
>
> > With the above change:
> > Acked-by: Jerin Jacob <jerinj@marvell.com>
> >
> >
> > >     ...
> > > };
> > >
> > > The non-zero value of rx_split_num field configures the receiving
> > > queue to split ingress packets into multiple segments to the mbufs
> > > allocated from various memory pools according to the specified
> > > lengths. The zero value of rx_split_num field provides the backward
> > > compatibility and queue should be configured in a regular way (with
> > > single/multiple mbufs of the same data buffer length allocated from
> > > the single memory pool).
> > >
> > > The new approach would allow splitting the ingress packets into
> > > multiple parts pushed to the memory with different attributes.
> > > For example, the packet headers can be pushed to the embedded data
> > > buffers within mbufs and the application data into the external
> > > buffers attached to mbufs allocated from the different memory pools.
> > > The memory attributes for the split parts may differ either - for
> > > example the application data may be pushed into the external memory
> > > located on the dedicated physical device, say GPU or NVMe. This would
> > > improve the DPDK receiving datapath flexibility preserving
> > > compatibility with existing API.
> > >
> > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > > ---
> > >  doc/guides/rel_notes/deprecation.rst | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > b/doc/guides/rel_notes/deprecation.rst
> > > index ea4cfa7..cd700ae 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -99,6 +99,11 @@ Deprecation Notices
> > >    In 19.11 PMDs will still update the field even when the offload is not
> > >    enabled.
> > >
> > > +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the
> > > +receiving
> > > +  queues to split ingress packets into multiple segments according to
> > > +the
> > > +  specified lengths into the buffers allocated from the specified
> > > +  memory pools. The backward compatibility to existing API is preserved.
> > > +
> > >  * ethdev: ``rx_descriptor_done`` dev_ops and
> > ``rte_eth_rx_descriptor_done``
> > >    will be deprecated in 20.11 and will be removed in 21.11.
> > >    Existing ``rte_eth_rx_descriptor_status`` and
> > > ``rte_eth_tx_descriptor_status``
> > > --
> > > 1.8.3.1
> > >
  
Slava Ovsiienko Aug. 5, 2020, 6:35 a.m. UTC | #5
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, August 4, 2020 16:33
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@mellanox.com>;
> Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Stephen
> Hemminger <stephen@networkplumber.org>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Olivier Matz <olivier.matz@6wind.com>;
> David Marchand <david.marchand@redhat.com>
> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> 
> On Mon, Aug 3, 2020 at 6:36 PM Slava Ovsiienko
> <viacheslavo@mellanox.com> wrote:
> >
> > Hi, Jerin,
> >
> > Thanks for the comment,  please, see below.
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Monday, August 3, 2020 14:57
> > > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@mellanox.com>;
> > > Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > > Stephen Hemminger <stephen@networkplumber.org>; Andrew
> Rybchenko
> > > <arybchenko@solarflare.com>; Ajit Khaparde
> > > <ajit.khaparde@broadcom.com>; Maxime Coquelin
> > > <maxime.coquelin@redhat.com>; Olivier Matz
> <olivier.matz@6wind.com>;
> > > David Marchand <david.marchand@redhat.com>
> > > Subject: Re: [PATCH] doc: announce changes to ethdev rxconf
> > > structure
> > >
> > > On Mon, Aug 3, 2020 at 4:28 PM Viacheslav Ovsiienko
> > > <viacheslavo@mellanox.com> wrote:
> > > >
> > > > The DPDK datapath in the transmit direction is very flexible.
> > > > The applications can build multisegment packets and manages almost
> > > > all data aspects - the memory pools where segments are allocated
> > > > from, the segment lengths, the memory attributes like external,
> registered, etc.
> > > >
> > > > In the receiving direction, the datapath is much less flexible,
> > > > the applications can only specify the memory pool to configure the
> > > > receiving queue and nothing more. In order to extend the receiving
> > > > datapath capabilities it is proposed to add the new fields into
> > > > rte_eth_rxconf structure:
> > > >
> > > > struct rte_eth_rxconf {
> > > >     ...
> > > >     uint16_t rx_split_num; /* number of segments to split */
> > > >     uint16_t *rx_split_len; /* array of segment lengthes */
> > > >     struct rte_mempool **mp; /* array of segment memory pools */
> > >
> > > The pool has the packet length it's been configured for.
> > > So I think, rx_split_len can be removed.
> >
> > Yes, it is one of the supposed options - if pointer to array of
> > segment lengths is NULL , the queue_setup() could use the lengths from the
> pool's properties.
> > But we are talking about packet split, in general, it should not
> > depend on pool properties. What if application provides the single
> > pool and just wants to have the tunnel header in the first dedicated mbuf?
> >
> > >
> > > This feature also available in Marvell HW. So it not specific to one
> vendor.
> > > Maybe we could just the use case mention the use case in the
> > > depreciation notice and the tentative change in rte_eth_rxconf and
> > > exact details can be worked out at the time of implementation.
> > >
> > So, if I understand correctly, the struct changes in the commit
> > message should be marked as just possible implementation?
> 
> Yes.
> 
> We may need to have a detailed discussion on the correct abstraction for
> various HW is available with this feature.
> 
> On Marvell HW, We can configure TWO pools for given eth Rx queue.
> One pool can be configured as a small packet pool and other one as large
> packet pool.
> And there is a threshold value to decide the pool between small and large.
> For example:
> - The small pool is configured 2k
> - The large pool is configured with 10k
> - And if the threshold value is configured as 2k.
> Any packet size <=2K will land in small pool and others in a large pool.
> The use case, we are targeting is to save the memory space for jumbo
> frames.

It is a little bit different than split feature, it is about somehow packet smart sorting.
"Buffer split" is just about more flexible description of rx buffers. Currently
the rx buffers can only be the chain of the buffers of the same size and
allocated from the same memory pool. It is simple and not versatile way,
we could extend this.

Of course, there is no any objection against sharing this split Rx buffer description
with other features, but, for the example above (2k/10k) it is only the part, it
would require some other parameters (threshold) not used by split. Yes, let's
discuss.

> 
> If you can share the MLX HW working model, Then we can find the correct
> abstraction.
From MLNX HW point of view buffer split feature does require nothing special.
The HW rx buffer descriptors support flexible buffer formats, there is
no problem to specify the chain of mbufs with different sizes and dedicated
pointers to receive and split packet into by hardware.


With best regards,
Slava
> >
> > > With the above change:
> > > Acked-by: Jerin Jacob <jerinj@marvell.com>
> > >
> > >
> > > >     ...
> > > > };
> > > >
> > > > The non-zero value of rx_split_num field configures the receiving
> > > > queue to split ingress packets into multiple segments to the mbufs
> > > > allocated from various memory pools according to the specified
> > > > lengths. The zero value of rx_split_num field provides the
> > > > backward compatibility and queue should be configured in a regular
> > > > way (with single/multiple mbufs of the same data buffer length
> > > > allocated from the single memory pool).
> > > >
> > > > The new approach would allow splitting the ingress packets into
> > > > multiple parts pushed to the memory with different attributes.
> > > > For example, the packet headers can be pushed to the embedded data
> > > > buffers within mbufs and the application data into the external
> > > > buffers attached to mbufs allocated from the different memory pools.
> > > > The memory attributes for the split parts may differ either - for
> > > > example the application data may be pushed into the external
> > > > memory located on the dedicated physical device, say GPU or NVMe.
> > > > This would improve the DPDK receiving datapath flexibility
> > > > preserving compatibility with existing API.
> > > >
> > > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > > > ---
> > > >  doc/guides/rel_notes/deprecation.rst | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > > b/doc/guides/rel_notes/deprecation.rst
> > > > index ea4cfa7..cd700ae 100644
> > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > @@ -99,6 +99,11 @@ Deprecation Notices
> > > >    In 19.11 PMDs will still update the field even when the offload is not
> > > >    enabled.
> > > >
> > > > +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the
> > > > +receiving
> > > > +  queues to split ingress packets into multiple segments
> > > > +according to the
> > > > +  specified lengths into the buffers allocated from the specified
> > > > +  memory pools. The backward compatibility to existing API is
> preserved.
> > > > +
> > > >  * ethdev: ``rx_descriptor_done`` dev_ops and
> > > ``rte_eth_rx_descriptor_done``
> > > >    will be deprecated in 20.11 and will be removed in 21.11.
> > > >    Existing ``rte_eth_rx_descriptor_status`` and
> > > > ``rte_eth_tx_descriptor_status``
> > > > --
> > > > 1.8.3.1
> > > >
  
Ferruh Yigit Aug. 6, 2020, 3:58 p.m. UTC | #6
On 8/4/2020 2:32 PM, Jerin Jacob wrote:
> On Mon, Aug 3, 2020 at 6:36 PM Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
>>
>> Hi, Jerin,
>>
>> Thanks for the comment,  please, see below.
>>
>>> -----Original Message-----
>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>> Sent: Monday, August 3, 2020 14:57
>>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
>>> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@mellanox.com>;
>>> Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon
>>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Stephen
>>> Hemminger <stephen@networkplumber.org>; Andrew Rybchenko
>>> <arybchenko@solarflare.com>; Ajit Khaparde
>>> <ajit.khaparde@broadcom.com>; Maxime Coquelin
>>> <maxime.coquelin@redhat.com>; Olivier Matz <olivier.matz@6wind.com>;
>>> David Marchand <david.marchand@redhat.com>
>>> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
>>>
>>> On Mon, Aug 3, 2020 at 4:28 PM Viacheslav Ovsiienko
>>> <viacheslavo@mellanox.com> wrote:
>>>>
>>>> The DPDK datapath in the transmit direction is very flexible.
>>>> The applications can build multisegment packets and manages almost all
>>>> data aspects - the memory pools where segments are allocated from, the
>>>> segment lengths, the memory attributes like external, registered, etc.
>>>>
>>>> In the receiving direction, the datapath is much less flexible, the
>>>> applications can only specify the memory pool to configure the
>>>> receiving queue and nothing more. In order to extend the receiving
>>>> datapath capabilities it is proposed to add the new fields into
>>>> rte_eth_rxconf structure:
>>>>
>>>> struct rte_eth_rxconf {
>>>>     ...
>>>>     uint16_t rx_split_num; /* number of segments to split */
>>>>     uint16_t *rx_split_len; /* array of segment lengthes */
>>>>     struct rte_mempool **mp; /* array of segment memory pools */
>>>
>>> The pool has the packet length it's been configured for.
>>> So I think, rx_split_len can be removed.
>>
>> Yes, it is one of the supposed options - if pointer to array of segment lengths
>> is NULL , the queue_setup() could use the lengths from the pool's properties.
>> But we are talking about packet split, in general, it should not depend
>> on pool properties. What if application provides the single pool
>> and just wants to have the tunnel header in the first dedicated mbuf?
>>
>>>
>>> This feature also available in Marvell HW. So it not specific to one vendor.
>>> Maybe we could just the use case mention the use case in the depreciation
>>> notice and the tentative change in rte_eth_rxconf and exact details can be
>>> worked out at the time of implementation.
>>>
>> So, if I understand correctly, the struct changes in the commit message
>> should be marked as just possible implementation?
> 
> Yes.
> 
> We may need to have a detailed discussion on the correct abstraction for various
> HW is available with this feature.
> 
> On Marvell HW, We can configure TWO pools for given eth Rx queue.
> One pool can be configured as a small packet pool and other one as
> large packet pool.
> And there is a threshold value to decide the pool between small and large.
> For example:
> - The small pool is configured 2k
> - The large pool is configured with 10k
> - And if the threshold value is configured as 2k.
> Any packet size <=2K will land in small pool and others in a large pool.
> The use case, we are targeting is to save the memory space for jumbo frames.

Out of curiosity, do you provide two different buffer address in the descriptor
and HW automatically uses one based on the size,
or driver uses one of the pools based on the configuration and possible largest
packet size?

> 
> If you can share the MLX HW working model, Then we can find the
> correct abstraction.
> 
>>
>> With best regards,
>> Slava
>>
>>> With the above change:
>>> Acked-by: Jerin Jacob <jerinj@marvell.com>
>>>
>>>
>>>>     ...
>>>> };
>>>>
>>>> The non-zero value of rx_split_num field configures the receiving
>>>> queue to split ingress packets into multiple segments to the mbufs
>>>> allocated from various memory pools according to the specified
>>>> lengths. The zero value of rx_split_num field provides the backward
>>>> compatibility and queue should be configured in a regular way (with
>>>> single/multiple mbufs of the same data buffer length allocated from
>>>> the single memory pool).
>>>>
>>>> The new approach would allow splitting the ingress packets into
>>>> multiple parts pushed to the memory with different attributes.
>>>> For example, the packet headers can be pushed to the embedded data
>>>> buffers within mbufs and the application data into the external
>>>> buffers attached to mbufs allocated from the different memory pools.
>>>> The memory attributes for the split parts may differ either - for
>>>> example the application data may be pushed into the external memory
>>>> located on the dedicated physical device, say GPU or NVMe. This would
>>>> improve the DPDK receiving datapath flexibility preserving
>>>> compatibility with existing API.
>>>>
>>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
>>>> ---
>>>>  doc/guides/rel_notes/deprecation.rst | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
>>>> b/doc/guides/rel_notes/deprecation.rst
>>>> index ea4cfa7..cd700ae 100644
>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>> @@ -99,6 +99,11 @@ Deprecation Notices
>>>>    In 19.11 PMDs will still update the field even when the offload is not
>>>>    enabled.
>>>>
>>>> +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the
>>>> +receiving
>>>> +  queues to split ingress packets into multiple segments according to
>>>> +the
>>>> +  specified lengths into the buffers allocated from the specified
>>>> +  memory pools. The backward compatibility to existing API is preserved.
>>>> +
>>>>  * ethdev: ``rx_descriptor_done`` dev_ops and
>>> ``rte_eth_rx_descriptor_done``
>>>>    will be deprecated in 20.11 and will be removed in 21.11.
>>>>    Existing ``rte_eth_rx_descriptor_status`` and
>>>> ``rte_eth_tx_descriptor_status``
>>>> --
>>>> 1.8.3.1
>>>>
  
Ferruh Yigit Aug. 6, 2020, 4:15 p.m. UTC | #7
On 8/3/2020 3:31 PM, Andrew Rybchenko wrote:
> On 8/3/20 1:58 PM, Viacheslav Ovsiienko wrote:
>> The DPDK datapath in the transmit direction is very flexible.
>> The applications can build multisegment packets and manages
>> almost all data aspects - the memory pools where segments
>> are allocated from, the segment lengths, the memory attributes
>> like external, registered, etc.
>>
>> In the receiving direction, the datapath is much less flexible,
>> the applications can only specify the memory pool to configure
>> the receiving queue and nothing more. In order to extend the
>> receiving datapath capabilities it is proposed to add the new
>> fields into rte_eth_rxconf structure:
>>
>> struct rte_eth_rxconf {
>>     ...
>>     uint16_t rx_split_num; /* number of segments to split */
>>     uint16_t *rx_split_len; /* array of segment lengthes */
>>     struct rte_mempool **mp; /* array of segment memory pools */
>>     ...
>> };
>>
>> The non-zero value of rx_split_num field configures the receiving
>> queue to split ingress packets into multiple segments to the mbufs
>> allocated from various memory pools according to the specified
>> lengths. The zero value of rx_split_num field provides the
>> backward compatibility and queue should be configured in a regular
>> way (with single/multiple mbufs of the same data buffer length
>> allocated from the single memory pool).
> 
> From the above description it is not 100% clear how it will
> coexist with:
>  - existing mb_pool argument of the rte_eth_rx_queue_setup()

+1

>  - DEV_RX_OFFLOAD_SCATTER
>  - DEV_RX_OFFLOAD_HEADER_SPLIT
> How will application know that the feature is supported? Limitations?

+1

> Is it always split by specified/fixed length?
> What happens if header length is actually different?

As far as I understand intention is to filter specific packets to a queue first
and later do the split, so the header length will be fixed...

> 
>> The new approach would allow splitting the ingress packets into
>> multiple parts pushed to the memory with different attributes.
>> For example, the packet headers can be pushed to the embedded data
>> buffers within mbufs and the application data into the external
>> buffers attached to mbufs allocated from the different memory
>> pools. The memory attributes for the split parts may differ
>> either - for example the application data may be pushed into
>> the external memory located on the dedicated physical device,
>> say GPU or NVMe. This would improve the DPDK receiving datapath
>> flexibility preserving compatibility with existing API.
>>
>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
>> ---
>>  doc/guides/rel_notes/deprecation.rst | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>> index ea4cfa7..cd700ae 100644
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> @@ -99,6 +99,11 @@ Deprecation Notices
>>    In 19.11 PMDs will still update the field even when the offload is not
>>    enabled.
>>  
>> +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the receiving
>> +  queues to split ingress packets into multiple segments according to the
>> +  specified lengths into the buffers allocated from the specified
>> +  memory pools. The backward compatibility to existing API is preserved.
>> +
>>  * ethdev: ``rx_descriptor_done`` dev_ops and ``rte_eth_rx_descriptor_done``
>>    will be deprecated in 20.11 and will be removed in 21.11.
>>    Existing ``rte_eth_rx_descriptor_status`` and ``rte_eth_tx_descriptor_status``
>
  
Stephen Hemminger Aug. 6, 2020, 4:25 p.m. UTC | #8
On Thu, 6 Aug 2020 16:58:22 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 8/4/2020 2:32 PM, Jerin Jacob wrote:
> > On Mon, Aug 3, 2020 at 6:36 PM Slava Ovsiienko <viacheslavo@mellanox.com> wrote:  
> >>
> >> Hi, Jerin,
> >>
> >> Thanks for the comment,  please, see below.
> >>  
> >>> -----Original Message-----
> >>> From: Jerin Jacob <jerinjacobk@gmail.com>
> >>> Sent: Monday, August 3, 2020 14:57
> >>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> >>> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@mellanox.com>;
> >>> Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon
> >>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Stephen
> >>> Hemminger <stephen@networkplumber.org>; Andrew Rybchenko
> >>> <arybchenko@solarflare.com>; Ajit Khaparde
> >>> <ajit.khaparde@broadcom.com>; Maxime Coquelin
> >>> <maxime.coquelin@redhat.com>; Olivier Matz <olivier.matz@6wind.com>;
> >>> David Marchand <david.marchand@redhat.com>
> >>> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> >>>
> >>> On Mon, Aug 3, 2020 at 4:28 PM Viacheslav Ovsiienko
> >>> <viacheslavo@mellanox.com> wrote:  
> >>>>
> >>>> The DPDK datapath in the transmit direction is very flexible.
> >>>> The applications can build multisegment packets and manages almost all
> >>>> data aspects - the memory pools where segments are allocated from, the
> >>>> segment lengths, the memory attributes like external, registered, etc.
> >>>>
> >>>> In the receiving direction, the datapath is much less flexible, the
> >>>> applications can only specify the memory pool to configure the
> >>>> receiving queue and nothing more. In order to extend the receiving
> >>>> datapath capabilities it is proposed to add the new fields into
> >>>> rte_eth_rxconf structure:
> >>>>
> >>>> struct rte_eth_rxconf {
> >>>>     ...
> >>>>     uint16_t rx_split_num; /* number of segments to split */
> >>>>     uint16_t *rx_split_len; /* array of segment lengthes */
> >>>>     struct rte_mempool **mp; /* array of segment memory pools */  
> >>>
> >>> The pool has the packet length it's been configured for.
> >>> So I think, rx_split_len can be removed.  
> >>
> >> Yes, it is one of the supposed options - if pointer to array of segment lengths
> >> is NULL , the queue_setup() could use the lengths from the pool's properties.
> >> But we are talking about packet split, in general, it should not depend
> >> on pool properties. What if application provides the single pool
> >> and just wants to have the tunnel header in the first dedicated mbuf?
> >>  
> >>>
> >>> This feature also available in Marvell HW. So it not specific to one vendor.
> >>> Maybe we could just the use case mention the use case in the depreciation
> >>> notice and the tentative change in rte_eth_rxconf and exact details can be
> >>> worked out at the time of implementation.
> >>>  
> >> So, if I understand correctly, the struct changes in the commit message
> >> should be marked as just possible implementation?  
> > 
> > Yes.
> > 
> > We may need to have a detailed discussion on the correct abstraction for various
> > HW is available with this feature.
> > 
> > On Marvell HW, We can configure TWO pools for given eth Rx queue.
> > One pool can be configured as a small packet pool and other one as
> > large packet pool.
> > And there is a threshold value to decide the pool between small and large.
> > For example:
> > - The small pool is configured 2k
> > - The large pool is configured with 10k
> > - And if the threshold value is configured as 2k.
> > Any packet size <=2K will land in small pool and others in a large pool.
> > The use case, we are targeting is to save the memory space for jumbo frames.  
> 
> Out of curiosity, do you provide two different buffer address in the descriptor
> and HW automatically uses one based on the size,
> or driver uses one of the pools based on the configuration and possible largest
> packet size?

I am all for allowing more configuration of buffer pool.
But don't want that to be exposed as a hardware specific requirement in the
API for applications. The worst case would be if your API changes required:

  if (strcmp(dev->driver_name, "marvell") == 0) {
     // make another mempool for this driver
  
Slava Ovsiienko Aug. 6, 2020, 4:29 p.m. UTC | #9
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, August 6, 2020 19:16
> To: Andrew Rybchenko <arybchenko@solarflare.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> jerinjacobk@gmail.com; stephen@networkplumber.org;
> ajit.khaparde@broadcom.com; maxime.coquelin@redhat.com;
> olivier.matz@6wind.com; david.marchand@redhat.com
> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> 
> On 8/3/2020 3:31 PM, Andrew Rybchenko wrote:
> > On 8/3/20 1:58 PM, Viacheslav Ovsiienko wrote:
> >> The DPDK datapath in the transmit direction is very flexible.
> >> The applications can build multisegment packets and manages almost
> >> all data aspects - the memory pools where segments are allocated
> >> from, the segment lengths, the memory attributes like external,
> >> registered, etc.
> >>
> >> In the receiving direction, the datapath is much less flexible, the
> >> applications can only specify the memory pool to configure the
> >> receiving queue and nothing more. In order to extend the receiving
> >> datapath capabilities it is proposed to add the new fields into
> >> rte_eth_rxconf structure:
> >>
> >> struct rte_eth_rxconf {
> >>     ...
> >>     uint16_t rx_split_num; /* number of segments to split */
> >>     uint16_t *rx_split_len; /* array of segment lengthes */
> >>     struct rte_mempool **mp; /* array of segment memory pools */
> >>     ...
> >> };
> >>
> >> The non-zero value of rx_split_num field configures the receiving
> >> queue to split ingress packets into multiple segments to the mbufs
> >> allocated from various memory pools according to the specified
> >> lengths. The zero value of rx_split_num field provides the backward
> >> compatibility and queue should be configured in a regular way (with
> >> single/multiple mbufs of the same data buffer length allocated from
> >> the single memory pool).
> >
> > From the above description it is not 100% clear how it will coexist
> > with:
> >  - existing mb_pool argument of the rte_eth_rx_queue_setup()
> 
> +1
- supposed to be NULL if the array of lengths/pools is used

> 
> >  - DEV_RX_OFFLOAD_SCATTER
> >  - DEV_RX_OFFLOAD_HEADER_SPLIT
> > How will application know that the feature is supported? Limitations?
> 
> +1
New flag  DEV_RX_OFFLOAD_BUFFER_SPLIT is supposed to be introduced.
The feature requires the DEV_RX_OFFLOAD_SCATTER is set.
If DEV_RX_OFFLOAD_HEADER_SPLIT is set the error is returned.

> 
> > Is it always split by specified/fixed length?
> > What happens if header length is actually different?
> 
> As far as I understand intention is to filter specific packets to a queue first
> and later do the split, so the header length will be fixed...

Not exactly. The filtering should be handled by rte_flow engine.
The intention is to provide the more flexible way to describe
rx buffers. Currently it is the single pool with fixed size segments. No way to
split the packet into multiple segments with specified lengths and in
the specified pools. What if packet payload should be stored in the physical
memory on other device (GPU/Storage)? What if caching is not desired for
the payload (just forwarding application)? We could provide the special NC pool.
What if packet should be split into the chunks with specific gaps?
For Tx direction we have this opportunity to gather packet from various
pools and any desired combinations , but Rx is much less flexible.
 
> >
> >> The new approach would allow splitting the ingress packets into
> >> multiple parts pushed to the memory with different attributes.
> >> For example, the packet headers can be pushed to the embedded data
> >> buffers within mbufs and the application data into the external
> >> buffers attached to mbufs allocated from the different memory pools.
> >> The memory attributes for the split parts may differ either - for
> >> example the application data may be pushed into the external memory
> >> located on the dedicated physical device, say GPU or NVMe. This would
> >> improve the DPDK receiving datapath flexibility preserving
> >> compatibility with existing API.
> >>
> >> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> >> ---
> >>  doc/guides/rel_notes/deprecation.rst | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/doc/guides/rel_notes/deprecation.rst
> >> b/doc/guides/rel_notes/deprecation.rst
> >> index ea4cfa7..cd700ae 100644
> >> --- a/doc/guides/rel_notes/deprecation.rst
> >> +++ b/doc/guides/rel_notes/deprecation.rst
> >> @@ -99,6 +99,11 @@ Deprecation Notices
> >>    In 19.11 PMDs will still update the field even when the offload is not
> >>    enabled.
> >>
> >> +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the
> >> +receiving
> >> +  queues to split ingress packets into multiple segments according
> >> +to the
> >> +  specified lengths into the buffers allocated from the specified
> >> +  memory pools. The backward compatibility to existing API is preserved.
> >> +
> >>  * ethdev: ``rx_descriptor_done`` dev_ops and
> ``rte_eth_rx_descriptor_done``
> >>    will be deprecated in 20.11 and will be removed in 21.11.
> >>    Existing ``rte_eth_rx_descriptor_status`` and
> >> ``rte_eth_tx_descriptor_status``
> >
  
Ferruh Yigit Aug. 6, 2020, 4:37 p.m. UTC | #10
On 8/6/2020 5:29 PM, Slava Ovsiienko wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, August 6, 2020 19:16
>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Slava Ovsiienko
>> <viacheslavo@mellanox.com>; dev@dpdk.org
>> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
>> <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
>> jerinjacobk@gmail.com; stephen@networkplumber.org;
>> ajit.khaparde@broadcom.com; maxime.coquelin@redhat.com;
>> olivier.matz@6wind.com; david.marchand@redhat.com
>> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
>>
>> On 8/3/2020 3:31 PM, Andrew Rybchenko wrote:
>>> On 8/3/20 1:58 PM, Viacheslav Ovsiienko wrote:
>>>> The DPDK datapath in the transmit direction is very flexible.
>>>> The applications can build multisegment packets and manages almost
>>>> all data aspects - the memory pools where segments are allocated
>>>> from, the segment lengths, the memory attributes like external,
>>>> registered, etc.
>>>>
>>>> In the receiving direction, the datapath is much less flexible, the
>>>> applications can only specify the memory pool to configure the
>>>> receiving queue and nothing more. In order to extend the receiving
>>>> datapath capabilities it is proposed to add the new fields into
>>>> rte_eth_rxconf structure:
>>>>
>>>> struct rte_eth_rxconf {
>>>>     ...
>>>>     uint16_t rx_split_num; /* number of segments to split */
>>>>     uint16_t *rx_split_len; /* array of segment lengthes */
>>>>     struct rte_mempool **mp; /* array of segment memory pools */
>>>>     ...
>>>> };
>>>>
>>>> The non-zero value of rx_split_num field configures the receiving
>>>> queue to split ingress packets into multiple segments to the mbufs
>>>> allocated from various memory pools according to the specified
>>>> lengths. The zero value of rx_split_num field provides the backward
>>>> compatibility and queue should be configured in a regular way (with
>>>> single/multiple mbufs of the same data buffer length allocated from
>>>> the single memory pool).
>>>
>>> From the above description it is not 100% clear how it will coexist
>>> with:
>>>  - existing mb_pool argument of the rte_eth_rx_queue_setup()
>>
>> +1
> - supposed to be NULL if the array of lengths/pools is used
> 
>>
>>>  - DEV_RX_OFFLOAD_SCATTER
>>>  - DEV_RX_OFFLOAD_HEADER_SPLIT
>>> How will application know that the feature is supported? Limitations?
>>
>> +1
> New flag  DEV_RX_OFFLOAD_BUFFER_SPLIT is supposed to be introduced.
> The feature requires the DEV_RX_OFFLOAD_SCATTER is set.
> If DEV_RX_OFFLOAD_HEADER_SPLIT is set the error is returned.
> 
>>
>>> Is it always split by specified/fixed length?
>>> What happens if header length is actually different?
>>
>> As far as I understand intention is to filter specific packets to a queue first
>> and later do the split, so the header length will be fixed...
> 
> Not exactly. The filtering should be handled by rte_flow engine.
> The intention is to provide the more flexible way to describe
> rx buffers. Currently it is the single pool with fixed size segments. No way to
> split the packet into multiple segments with specified lengths and in
> the specified pools. What if packet payload should be stored in the physical
> memory on other device (GPU/Storage)? What if caching is not desired for
> the payload (just forwarding application)? We could provide the special NC pool.
> What if packet should be split into the chunks with specific gaps?
> For Tx direction we have this opportunity to gather packet from various
> pools and any desired combinations , but Rx is much less flexible.
>  
>>>
>>>> The new approach would allow splitting the ingress packets into
>>>> multiple parts pushed to the memory with different attributes.
>>>> For example, the packet headers can be pushed to the embedded data
>>>> buffers within mbufs and the application data into the external
>>>> buffers attached to mbufs allocated from the different memory pools.
>>>> The memory attributes for the split parts may differ either - for
>>>> example the application data may be pushed into the external memory
>>>> located on the dedicated physical device, say GPU or NVMe. This would
>>>> improve the DPDK receiving datapath flexibility preserving
>>>> compatibility with existing API.

If you don't know the packet types in advance, how can you use fixed sizes to
split a packet? Won't it be like having random parts of packet in each mempool..

>>>>
>>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
>>>> ---
>>>>  doc/guides/rel_notes/deprecation.rst | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
>>>> b/doc/guides/rel_notes/deprecation.rst
>>>> index ea4cfa7..cd700ae 100644
>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>> @@ -99,6 +99,11 @@ Deprecation Notices
>>>>    In 19.11 PMDs will still update the field even when the offload is not
>>>>    enabled.
>>>>
>>>> +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the
>>>> +receiving
>>>> +  queues to split ingress packets into multiple segments according
>>>> +to the
>>>> +  specified lengths into the buffers allocated from the specified
>>>> +  memory pools. The backward compatibility to existing API is preserved.
>>>> +
>>>>  * ethdev: ``rx_descriptor_done`` dev_ops and
>> ``rte_eth_rx_descriptor_done``
>>>>    will be deprecated in 20.11 and will be removed in 21.11.
>>>>    Existing ``rte_eth_rx_descriptor_status`` and
>>>> ``rte_eth_tx_descriptor_status``
>>>
>
  
Slava Ovsiienko Aug. 6, 2020, 4:39 p.m. UTC | #11
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, August 6, 2020 19:37
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> jerinjacobk@gmail.com; stephen@networkplumber.org;
> ajit.khaparde@broadcom.com; maxime.coquelin@redhat.com;
> olivier.matz@6wind.com; david.marchand@redhat.com
> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> 
> On 8/6/2020 5:29 PM, Slava Ovsiienko wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Thursday, August 6, 2020 19:16
> >> To: Andrew Rybchenko <arybchenko@solarflare.com>; Slava Ovsiienko
> >> <viacheslavo@mellanox.com>; dev@dpdk.org
> >> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> >> <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> >> jerinjacobk@gmail.com; stephen@networkplumber.org;
> >> ajit.khaparde@broadcom.com; maxime.coquelin@redhat.com;
> >> olivier.matz@6wind.com; david.marchand@redhat.com
> >> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> >>
> >> On 8/3/2020 3:31 PM, Andrew Rybchenko wrote:
> >>> On 8/3/20 1:58 PM, Viacheslav Ovsiienko wrote:
> >>>> The DPDK datapath in the transmit direction is very flexible.
> >>>> The applications can build multisegment packets and manages almost
> >>>> all data aspects - the memory pools where segments are allocated
> >>>> from, the segment lengths, the memory attributes like external,
> >>>> registered, etc.
> >>>>
> >>>> In the receiving direction, the datapath is much less flexible, the
> >>>> applications can only specify the memory pool to configure the
> >>>> receiving queue and nothing more. In order to extend the receiving
> >>>> datapath capabilities it is proposed to add the new fields into
> >>>> rte_eth_rxconf structure:
> >>>>
> >>>> struct rte_eth_rxconf {
> >>>>     ...
> >>>>     uint16_t rx_split_num; /* number of segments to split */
> >>>>     uint16_t *rx_split_len; /* array of segment lengthes */
> >>>>     struct rte_mempool **mp; /* array of segment memory pools */
> >>>>     ...
> >>>> };
> >>>>
> >>>> The non-zero value of rx_split_num field configures the receiving
> >>>> queue to split ingress packets into multiple segments to the mbufs
> >>>> allocated from various memory pools according to the specified
> >>>> lengths. The zero value of rx_split_num field provides the backward
> >>>> compatibility and queue should be configured in a regular way (with
> >>>> single/multiple mbufs of the same data buffer length allocated from
> >>>> the single memory pool).
> >>>
> >>> From the above description it is not 100% clear how it will coexist
> >>> with:
> >>>  - existing mb_pool argument of the rte_eth_rx_queue_setup()
> >>
> >> +1
> > - supposed to be NULL if the array of lengths/pools is used
> >
> >>
> >>>  - DEV_RX_OFFLOAD_SCATTER
> >>>  - DEV_RX_OFFLOAD_HEADER_SPLIT
> >>> How will application know that the feature is supported? Limitations?
> >>
> >> +1
> > New flag  DEV_RX_OFFLOAD_BUFFER_SPLIT is supposed to be introduced.
> > The feature requires the DEV_RX_OFFLOAD_SCATTER is set.
> > If DEV_RX_OFFLOAD_HEADER_SPLIT is set the error is returned.
> >
> >>
> >>> Is it always split by specified/fixed length?
> >>> What happens if header length is actually different?
> >>
> >> As far as I understand intention is to filter specific packets to a
> >> queue first and later do the split, so the header length will be fixed...
> >
> > Not exactly. The filtering should be handled by rte_flow engine.
> > The intention is to provide the more flexible way to describe rx
> > buffers. Currently it is the single pool with fixed size segments. No
> > way to split the packet into multiple segments with specified lengths
> > and in the specified pools. What if packet payload should be stored in
> > the physical memory on other device (GPU/Storage)? What if caching is
> > not desired for the payload (just forwarding application)? We could provide
> the special NC pool.
> > What if packet should be split into the chunks with specific gaps?
> > For Tx direction we have this opportunity to gather packet from
> > various pools and any desired combinations , but Rx is much less flexible.
> >
> >>>
> >>>> The new approach would allow splitting the ingress packets into
> >>>> multiple parts pushed to the memory with different attributes.
> >>>> For example, the packet headers can be pushed to the embedded data
> >>>> buffers within mbufs and the application data into the external
> >>>> buffers attached to mbufs allocated from the different memory pools.
> >>>> The memory attributes for the split parts may differ either - for
> >>>> example the application data may be pushed into the external memory
> >>>> located on the dedicated physical device, say GPU or NVMe. This
> >>>> would improve the DPDK receiving datapath flexibility preserving
> >>>> compatibility with existing API.
> 
> If you don't know the packet types in advance, how can you use fixed sizes to
> split a packet? Won't it be like having random parts of packet in each
> mempool..
It is per queue configuration. We have the rte_flow engine and can filter out
the desired packets to the desired queue.

> 
> >>>>
> >>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> >>>> ---
> >>>>  doc/guides/rel_notes/deprecation.rst | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/doc/guides/rel_notes/deprecation.rst
> >>>> b/doc/guides/rel_notes/deprecation.rst
> >>>> index ea4cfa7..cd700ae 100644
> >>>> --- a/doc/guides/rel_notes/deprecation.rst
> >>>> +++ b/doc/guides/rel_notes/deprecation.rst
> >>>> @@ -99,6 +99,11 @@ Deprecation Notices
> >>>>    In 19.11 PMDs will still update the field even when the offload is not
> >>>>    enabled.
> >>>>
> >>>> +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the
> >>>> +receiving
> >>>> +  queues to split ingress packets into multiple segments according
> >>>> +to the
> >>>> +  specified lengths into the buffers allocated from the specified
> >>>> +  memory pools. The backward compatibility to existing API is
> preserved.
> >>>> +
> >>>>  * ethdev: ``rx_descriptor_done`` dev_ops and
> >> ``rte_eth_rx_descriptor_done``
> >>>>    will be deprecated in 20.11 and will be removed in 21.11.
> >>>>    Existing ``rte_eth_rx_descriptor_status`` and
> >>>> ``rte_eth_tx_descriptor_status``
> >>>
> >
  
Jerin Jacob Aug. 6, 2020, 4:41 p.m. UTC | #12
On Thu, Aug 6, 2020 at 9:56 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Thu, 6 Aug 2020 16:58:22 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> > On 8/4/2020 2:32 PM, Jerin Jacob wrote:
> > > On Mon, Aug 3, 2020 at 6:36 PM Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> > >>
> > >> Hi, Jerin,
> > >>
> > >> Thanks for the comment,  please, see below.
> > >>
> > >>> -----Original Message-----
> > >>> From: Jerin Jacob <jerinjacobk@gmail.com>
> > >>> Sent: Monday, August 3, 2020 14:57
> > >>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > >>> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@mellanox.com>;
> > >>> Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon
> > >>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Stephen
> > >>> Hemminger <stephen@networkplumber.org>; Andrew Rybchenko
> > >>> <arybchenko@solarflare.com>; Ajit Khaparde
> > >>> <ajit.khaparde@broadcom.com>; Maxime Coquelin
> > >>> <maxime.coquelin@redhat.com>; Olivier Matz <olivier.matz@6wind.com>;
> > >>> David Marchand <david.marchand@redhat.com>
> > >>> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> > >>>
> > >>> On Mon, Aug 3, 2020 at 4:28 PM Viacheslav Ovsiienko
> > >>> <viacheslavo@mellanox.com> wrote:
> > >>>>
> > >>>> The DPDK datapath in the transmit direction is very flexible.
> > >>>> The applications can build multisegment packets and manages almost all
> > >>>> data aspects - the memory pools where segments are allocated from, the
> > >>>> segment lengths, the memory attributes like external, registered, etc.
> > >>>>
> > >>>> In the receiving direction, the datapath is much less flexible, the
> > >>>> applications can only specify the memory pool to configure the
> > >>>> receiving queue and nothing more. In order to extend the receiving
> > >>>> datapath capabilities it is proposed to add the new fields into
> > >>>> rte_eth_rxconf structure:
> > >>>>
> > >>>> struct rte_eth_rxconf {
> > >>>>     ...
> > >>>>     uint16_t rx_split_num; /* number of segments to split */
> > >>>>     uint16_t *rx_split_len; /* array of segment lengthes */
> > >>>>     struct rte_mempool **mp; /* array of segment memory pools */
> > >>>
> > >>> The pool has the packet length it's been configured for.
> > >>> So I think, rx_split_len can be removed.
> > >>
> > >> Yes, it is one of the supposed options - if pointer to array of segment lengths
> > >> is NULL , the queue_setup() could use the lengths from the pool's properties.
> > >> But we are talking about packet split, in general, it should not depend
> > >> on pool properties. What if application provides the single pool
> > >> and just wants to have the tunnel header in the first dedicated mbuf?
> > >>
> > >>>
> > >>> This feature also available in Marvell HW. So it not specific to one vendor.
> > >>> Maybe we could just the use case mention the use case in the depreciation
> > >>> notice and the tentative change in rte_eth_rxconf and exact details can be
> > >>> worked out at the time of implementation.
> > >>>
> > >> So, if I understand correctly, the struct changes in the commit message
> > >> should be marked as just possible implementation?
> > >
> > > Yes.
> > >
> > > We may need to have a detailed discussion on the correct abstraction for various
> > > HW is available with this feature.
> > >
> > > On Marvell HW, We can configure TWO pools for given eth Rx queue.
> > > One pool can be configured as a small packet pool and other one as
> > > large packet pool.
> > > And there is a threshold value to decide the pool between small and large.
> > > For example:
> > > - The small pool is configured 2k
> > > - The large pool is configured with 10k
> > > - And if the threshold value is configured as 2k.
> > > Any packet size <=2K will land in small pool and others in a large pool.
> > > The use case, we are targeting is to save the memory space for jumbo frames.
> >
> > Out of curiosity, do you provide two different buffer address in the descriptor
> > and HW automatically uses one based on the size,
> > or driver uses one of the pools based on the configuration and possible largest
> > packet size?

The later one.

>
> I am all for allowing more configuration of buffer pool.
> But don't want that to be exposed as a hardware specific requirement in the
> API for applications. The worst case would be if your API changes required:
>
>   if (strcmp(dev->driver_name, "marvell") == 0) {
>      // make another mempool for this driver

There is no HW specific requirements here. If one pool specified(like
the existing situation),
HW will create scatter-gather frame.

It is mostly useful for the application use case where it needs single
contiguous of data
for processing(like crypto) and/or improving Rx/TX performance by
running in single seg mode
without losing too much of memory.


>
>
  
Ferruh Yigit Aug. 6, 2020, 4:43 p.m. UTC | #13
On 8/6/2020 5:39 PM, Slava Ovsiienko wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, August 6, 2020 19:37
>> To: Slava Ovsiienko <viacheslavo@mellanox.com>; Andrew Rybchenko
>> <arybchenko@solarflare.com>; dev@dpdk.org
>> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
>> <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
>> jerinjacobk@gmail.com; stephen@networkplumber.org;
>> ajit.khaparde@broadcom.com; maxime.coquelin@redhat.com;
>> olivier.matz@6wind.com; david.marchand@redhat.com
>> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
>>
>> On 8/6/2020 5:29 PM, Slava Ovsiienko wrote:
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Sent: Thursday, August 6, 2020 19:16
>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Slava Ovsiienko
>>>> <viacheslavo@mellanox.com>; dev@dpdk.org
>>>> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
>>>> <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
>>>> jerinjacobk@gmail.com; stephen@networkplumber.org;
>>>> ajit.khaparde@broadcom.com; maxime.coquelin@redhat.com;
>>>> olivier.matz@6wind.com; david.marchand@redhat.com
>>>> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
>>>>
>>>> On 8/3/2020 3:31 PM, Andrew Rybchenko wrote:
>>>>> On 8/3/20 1:58 PM, Viacheslav Ovsiienko wrote:
>>>>>> The DPDK datapath in the transmit direction is very flexible.
>>>>>> The applications can build multisegment packets and manages almost
>>>>>> all data aspects - the memory pools where segments are allocated
>>>>>> from, the segment lengths, the memory attributes like external,
>>>>>> registered, etc.
>>>>>>
>>>>>> In the receiving direction, the datapath is much less flexible, the
>>>>>> applications can only specify the memory pool to configure the
>>>>>> receiving queue and nothing more. In order to extend the receiving
>>>>>> datapath capabilities it is proposed to add the new fields into
>>>>>> rte_eth_rxconf structure:
>>>>>>
>>>>>> struct rte_eth_rxconf {
>>>>>>     ...
>>>>>>     uint16_t rx_split_num; /* number of segments to split */
>>>>>>     uint16_t *rx_split_len; /* array of segment lengthes */
>>>>>>     struct rte_mempool **mp; /* array of segment memory pools */
>>>>>>     ...
>>>>>> };
>>>>>>
>>>>>> The non-zero value of rx_split_num field configures the receiving
>>>>>> queue to split ingress packets into multiple segments to the mbufs
>>>>>> allocated from various memory pools according to the specified
>>>>>> lengths. The zero value of rx_split_num field provides the backward
>>>>>> compatibility and queue should be configured in a regular way (with
>>>>>> single/multiple mbufs of the same data buffer length allocated from
>>>>>> the single memory pool).
>>>>>
>>>>> From the above description it is not 100% clear how it will coexist
>>>>> with:
>>>>>  - existing mb_pool argument of the rte_eth_rx_queue_setup()
>>>>
>>>> +1
>>> - supposed to be NULL if the array of lengths/pools is used
>>>
>>>>
>>>>>  - DEV_RX_OFFLOAD_SCATTER
>>>>>  - DEV_RX_OFFLOAD_HEADER_SPLIT
>>>>> How will application know that the feature is supported? Limitations?
>>>>
>>>> +1
>>> New flag  DEV_RX_OFFLOAD_BUFFER_SPLIT is supposed to be introduced.
>>> The feature requires the DEV_RX_OFFLOAD_SCATTER is set.
>>> If DEV_RX_OFFLOAD_HEADER_SPLIT is set the error is returned.
>>>
>>>>
>>>>> Is it always split by specified/fixed length?
>>>>> What happens if header length is actually different?
>>>>
>>>> As far as I understand intention is to filter specific packets to a
>>>> queue first and later do the split, so the header length will be fixed...
>>>
>>> Not exactly. The filtering should be handled by rte_flow engine.
>>> The intention is to provide the more flexible way to describe rx
>>> buffers. Currently it is the single pool with fixed size segments. No
>>> way to split the packet into multiple segments with specified lengths
>>> and in the specified pools. What if packet payload should be stored in
>>> the physical memory on other device (GPU/Storage)? What if caching is
>>> not desired for the payload (just forwarding application)? We could provide
>> the special NC pool.
>>> What if packet should be split into the chunks with specific gaps?
>>> For Tx direction we have this opportunity to gather packet from
>>> various pools and any desired combinations , but Rx is much less flexible.
>>>
>>>>>
>>>>>> The new approach would allow splitting the ingress packets into
>>>>>> multiple parts pushed to the memory with different attributes.
>>>>>> For example, the packet headers can be pushed to the embedded data
>>>>>> buffers within mbufs and the application data into the external
>>>>>> buffers attached to mbufs allocated from the different memory pools.
>>>>>> The memory attributes for the split parts may differ either - for
>>>>>> example the application data may be pushed into the external memory
>>>>>> located on the dedicated physical device, say GPU or NVMe. This
>>>>>> would improve the DPDK receiving datapath flexibility preserving
>>>>>> compatibility with existing API.
>>
>> If you don't know the packet types in advance, how can you use fixed sizes to
>> split a packet? Won't it be like having random parts of packet in each
>> mempool..
> It is per queue configuration. We have the rte_flow engine and can filter out
> the desired packets to the desired queue.

That is what I was trying to say above, intentions is first filter the packets
to a specific queue, later split them into multiple mempools, you said "not
exactly", what is the difference I am missing?

> 
>>
>>>>>>
>>>>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
>>>>>> ---
>>>>>>  doc/guides/rel_notes/deprecation.rst | 5 +++++
>>>>>>  1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
>>>>>> b/doc/guides/rel_notes/deprecation.rst
>>>>>> index ea4cfa7..cd700ae 100644
>>>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>>>> @@ -99,6 +99,11 @@ Deprecation Notices
>>>>>>    In 19.11 PMDs will still update the field even when the offload is not
>>>>>>    enabled.
>>>>>>
>>>>>> +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the
>>>>>> +receiving
>>>>>> +  queues to split ingress packets into multiple segments according
>>>>>> +to the
>>>>>> +  specified lengths into the buffers allocated from the specified
>>>>>> +  memory pools. The backward compatibility to existing API is
>> preserved.
>>>>>> +
>>>>>>  * ethdev: ``rx_descriptor_done`` dev_ops and
>>>> ``rte_eth_rx_descriptor_done``
>>>>>>    will be deprecated in 20.11 and will be removed in 21.11.
>>>>>>    Existing ``rte_eth_rx_descriptor_status`` and
>>>>>> ``rte_eth_tx_descriptor_status``
>>>>>
>>>
>
  
Slava Ovsiienko Aug. 6, 2020, 4:48 p.m. UTC | #14
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, August 6, 2020 19:43
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> jerinjacobk@gmail.com; stephen@networkplumber.org;
> ajit.khaparde@broadcom.com; maxime.coquelin@redhat.com;
> olivier.matz@6wind.com; david.marchand@redhat.com
> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> 
> On 8/6/2020 5:39 PM, Slava Ovsiienko wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Thursday, August 6, 2020 19:37
> >> To: Slava Ovsiienko <viacheslavo@mellanox.com>; Andrew Rybchenko
> >> <arybchenko@solarflare.com>; dev@dpdk.org
> >> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> >> <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> >> jerinjacobk@gmail.com; stephen@networkplumber.org;
> >> ajit.khaparde@broadcom.com; maxime.coquelin@redhat.com;
> >> olivier.matz@6wind.com; david.marchand@redhat.com
> >> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> >>
> >> On 8/6/2020 5:29 PM, Slava Ovsiienko wrote:
> >>>> -----Original Message-----
> >>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>> Sent: Thursday, August 6, 2020 19:16
> >>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Slava Ovsiienko
> >>>> <viacheslavo@mellanox.com>; dev@dpdk.org
> >>>> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> >>>> <rasland@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>;
> >>>> jerinjacobk@gmail.com; stephen@networkplumber.org;
> >>>> ajit.khaparde@broadcom.com; maxime.coquelin@redhat.com;
> >>>> olivier.matz@6wind.com; david.marchand@redhat.com
> >>>> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf
> >>>> structure
> >>>>
> >>>> On 8/3/2020 3:31 PM, Andrew Rybchenko wrote:
> >>>>> On 8/3/20 1:58 PM, Viacheslav Ovsiienko wrote:
> >>>>>> The DPDK datapath in the transmit direction is very flexible.
> >>>>>> The applications can build multisegment packets and manages
> >>>>>> almost all data aspects - the memory pools where segments are
> >>>>>> allocated from, the segment lengths, the memory attributes like
> >>>>>> external, registered, etc.
> >>>>>>
> >>>>>> In the receiving direction, the datapath is much less flexible,
> >>>>>> the applications can only specify the memory pool to configure
> >>>>>> the receiving queue and nothing more. In order to extend the
> >>>>>> receiving datapath capabilities it is proposed to add the new
> >>>>>> fields into rte_eth_rxconf structure:
> >>>>>>
> >>>>>> struct rte_eth_rxconf {
> >>>>>>     ...
> >>>>>>     uint16_t rx_split_num; /* number of segments to split */
> >>>>>>     uint16_t *rx_split_len; /* array of segment lengthes */
> >>>>>>     struct rte_mempool **mp; /* array of segment memory pools */
> >>>>>>     ...
> >>>>>> };
> >>>>>>
> >>>>>> The non-zero value of rx_split_num field configures the receiving
> >>>>>> queue to split ingress packets into multiple segments to the
> >>>>>> mbufs allocated from various memory pools according to the
> >>>>>> specified lengths. The zero value of rx_split_num field provides
> >>>>>> the backward compatibility and queue should be configured in a
> >>>>>> regular way (with single/multiple mbufs of the same data buffer
> >>>>>> length allocated from the single memory pool).
> >>>>>
> >>>>> From the above description it is not 100% clear how it will
> >>>>> coexist
> >>>>> with:
> >>>>>  - existing mb_pool argument of the rte_eth_rx_queue_setup()
> >>>>
> >>>> +1
> >>> - supposed to be NULL if the array of lengths/pools is used
> >>>
> >>>>
> >>>>>  - DEV_RX_OFFLOAD_SCATTER
> >>>>>  - DEV_RX_OFFLOAD_HEADER_SPLIT
> >>>>> How will application know that the feature is supported? Limitations?
> >>>>
> >>>> +1
> >>> New flag  DEV_RX_OFFLOAD_BUFFER_SPLIT is supposed to be
> introduced.
> >>> The feature requires the DEV_RX_OFFLOAD_SCATTER is set.
> >>> If DEV_RX_OFFLOAD_HEADER_SPLIT is set the error is returned.
> >>>
> >>>>
> >>>>> Is it always split by specified/fixed length?
> >>>>> What happens if header length is actually different?
> >>>>
> >>>> As far as I understand intention is to filter specific packets to a
> >>>> queue first and later do the split, so the header length will be fixed...
> >>>
> >>> Not exactly. The filtering should be handled by rte_flow engine.
> >>> The intention is to provide the more flexible way to describe rx
> >>> buffers. Currently it is the single pool with fixed size segments.
> >>> No way to split the packet into multiple segments with specified
> >>> lengths and in the specified pools. What if packet payload should be
> >>> stored in the physical memory on other device (GPU/Storage)? What if
> >>> caching is not desired for the payload (just forwarding
> >>> application)? We could provide
> >> the special NC pool.
> >>> What if packet should be split into the chunks with specific gaps?
> >>> For Tx direction we have this opportunity to gather packet from
> >>> various pools and any desired combinations , but Rx is much less flexible.
> >>>
> >>>>>
> >>>>>> The new approach would allow splitting the ingress packets into
> >>>>>> multiple parts pushed to the memory with different attributes.
> >>>>>> For example, the packet headers can be pushed to the embedded
> >>>>>> data buffers within mbufs and the application data into the
> >>>>>> external buffers attached to mbufs allocated from the different
> memory pools.
> >>>>>> The memory attributes for the split parts may differ either - for
> >>>>>> example the application data may be pushed into the external
> >>>>>> memory located on the dedicated physical device, say GPU or NVMe.
> >>>>>> This would improve the DPDK receiving datapath flexibility
> >>>>>> preserving compatibility with existing API.
> >>
> >> If you don't know the packet types in advance, how can you use fixed
> >> sizes to split a packet? Won't it be like having random parts of
> >> packet in each mempool..
> > It is per queue configuration. We have the rte_flow engine and can
> > filter out the desired packets to the desired queue.
> 
> That is what I was trying to say above, intentions is first filter the packets to
> a specific queue, later split them into multiple mempools, you said "not
> exactly", what is the difference I am missing?

Sorry, it is my bad - I mixed up with Marvell's queue capability to sort packets into two
pools depending on packet size. Yes, you are completely correct, first filter out the specific packets
to the dedicated queue and then split ones into the chunks of specified fixed sizes.
> 
> >
> >>
> >>>>>>
> >>>>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> >>>>>> ---
> >>>>>>  doc/guides/rel_notes/deprecation.rst | 5 +++++
> >>>>>>  1 file changed, 5 insertions(+)
> >>>>>>
> >>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
> >>>>>> b/doc/guides/rel_notes/deprecation.rst
> >>>>>> index ea4cfa7..cd700ae 100644
> >>>>>> --- a/doc/guides/rel_notes/deprecation.rst
> >>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
> >>>>>> @@ -99,6 +99,11 @@ Deprecation Notices
> >>>>>>    In 19.11 PMDs will still update the field even when the offload is
> not
> >>>>>>    enabled.
> >>>>>>
> >>>>>> +* ethdev: add new fields to ``rte_eth_rxconf`` to configure the
> >>>>>> +receiving
> >>>>>> +  queues to split ingress packets into multiple segments
> >>>>>> +according to the
> >>>>>> +  specified lengths into the buffers allocated from the
> >>>>>> +specified
> >>>>>> +  memory pools. The backward compatibility to existing API is
> >> preserved.
> >>>>>> +
> >>>>>>  * ethdev: ``rx_descriptor_done`` dev_ops and
> >>>> ``rte_eth_rx_descriptor_done``
> >>>>>>    will be deprecated in 20.11 and will be removed in 21.11.
> >>>>>>    Existing ``rte_eth_rx_descriptor_status`` and
> >>>>>> ``rte_eth_tx_descriptor_status``
> >>>>>
> >>>
> >
  
Slava Ovsiienko Aug. 6, 2020, 5:03 p.m. UTC | #15
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, August 6, 2020 19:26
> To: Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; dpdk-dev <dev@dpdk.org>; Matan Azrad
> <matan@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> Thomas Monjalon <thomas@monjalon.net>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Olivier Matz <olivier.matz@6wind.com>;
> David Marchand <david.marchand@redhat.com>
> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> 
> On Thu, 6 Aug 2020 16:58:22 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 8/4/2020 2:32 PM, Jerin Jacob wrote:
> > > On Mon, Aug 3, 2020 at 6:36 PM Slava Ovsiienko
> <viacheslavo@mellanox.com> wrote:
> > >>
> > >> Hi, Jerin,
> > >>
> > >> Thanks for the comment,  please, see below.
> > >>
> > >>> -----Original Message-----
> > >>> From: Jerin Jacob <jerinjacobk@gmail.com>
> > >>> Sent: Monday, August 3, 2020 14:57
> > >>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > >>> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@mellanox.com>;
> > >>> Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon
> > >>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > >>> Stephen Hemminger <stephen@networkplumber.org>; Andrew
> Rybchenko
> > >>> <arybchenko@solarflare.com>; Ajit Khaparde
> > >>> <ajit.khaparde@broadcom.com>; Maxime Coquelin
> > >>> <maxime.coquelin@redhat.com>; Olivier Matz
> > >>> <olivier.matz@6wind.com>; David Marchand
> > >>> <david.marchand@redhat.com>
> > >>> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf
> > >>> structure
> > >>>
> > >>> On Mon, Aug 3, 2020 at 4:28 PM Viacheslav Ovsiienko
> > >>> <viacheslavo@mellanox.com> wrote:
> > >>>>
> > >>>> The DPDK datapath in the transmit direction is very flexible.
> > >>>> The applications can build multisegment packets and manages
> > >>>> almost all data aspects - the memory pools where segments are
> > >>>> allocated from, the segment lengths, the memory attributes like
> external, registered, etc.
> > >>>>
> > >>>> In the receiving direction, the datapath is much less flexible,
> > >>>> the applications can only specify the memory pool to configure
> > >>>> the receiving queue and nothing more. In order to extend the
> > >>>> receiving datapath capabilities it is proposed to add the new
> > >>>> fields into rte_eth_rxconf structure:
> > >>>>
> > >>>> struct rte_eth_rxconf {
> > >>>>     ...
> > >>>>     uint16_t rx_split_num; /* number of segments to split */
> > >>>>     uint16_t *rx_split_len; /* array of segment lengthes */
> > >>>>     struct rte_mempool **mp; /* array of segment memory pools */
> > >>>
> > >>> The pool has the packet length it's been configured for.
> > >>> So I think, rx_split_len can be removed.
> > >>
> > >> Yes, it is one of the supposed options - if pointer to array of
> > >> segment lengths is NULL , the queue_setup() could use the lengths from
> the pool's properties.
> > >> But we are talking about packet split, in general, it should not
> > >> depend on pool properties. What if application provides the single
> > >> pool and just wants to have the tunnel header in the first dedicated
> mbuf?
> > >>
> > >>>
> > >>> This feature also available in Marvell HW. So it not specific to one
> vendor.
> > >>> Maybe we could just the use case mention the use case in the
> > >>> depreciation notice and the tentative change in rte_eth_rxconf and
> > >>> exact details can be worked out at the time of implementation.
> > >>>
> > >> So, if I understand correctly, the struct changes in the commit
> > >> message should be marked as just possible implementation?
> > >
> > > Yes.
> > >
> > > We may need to have a detailed discussion on the correct abstraction
> > > for various HW is available with this feature.
> > >
> > > On Marvell HW, We can configure TWO pools for given eth Rx queue.
> > > One pool can be configured as a small packet pool and other one as
> > > large packet pool.
> > > And there is a threshold value to decide the pool between small and
> large.
> > > For example:
> > > - The small pool is configured 2k
> > > - The large pool is configured with 10k
> > > - And if the threshold value is configured as 2k.
> > > Any packet size <=2K will land in small pool and others in a large pool.
> > > The use case, we are targeting is to save the memory space for jumbo
> frames.
> >
> > Out of curiosity, do you provide two different buffer address in the
> > descriptor and HW automatically uses one based on the size, or driver
> > uses one of the pools based on the configuration and possible largest
> > packet size?
> 
> I am all for allowing more configuration of buffer pool.
> But don't want that to be exposed as a hardware specific requirement in the
> API for applications. The worst case would be if your API changes required:
> 
>   if (strcmp(dev->driver_name, "marvell") == 0) {
>      // make another mempool for this driver
> 
I thought about adding some other segment attributes, vendor specific.
We could describe the segments with some descriptor structure (size, pool)
and add flags field to one. The proposals from other vendors are welcome.
  
Stephen Hemminger Aug. 6, 2020, 6:10 p.m. UTC | #16
On Thu, 6 Aug 2020 17:03:31 +0000
Slava Ovsiienko <viacheslavo@mellanox.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Thursday, August 6, 2020 19:26
> > To: Ferruh Yigit <ferruh.yigit@intel.com>
> > Cc: Jerin Jacob <jerinjacobk@gmail.com>; Slava Ovsiienko
> > <viacheslavo@mellanox.com>; dpdk-dev <dev@dpdk.org>; Matan Azrad
> > <matan@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> > Thomas Monjalon <thomas@monjalon.net>; Andrew Rybchenko
> > <arybchenko@solarflare.com>; Ajit Khaparde
> > <ajit.khaparde@broadcom.com>; Maxime Coquelin
> > <maxime.coquelin@redhat.com>; Olivier Matz <olivier.matz@6wind.com>;
> > David Marchand <david.marchand@redhat.com>
> > Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> > 
> > On Thu, 6 Aug 2020 16:58:22 +0100
> > Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >   
> > > On 8/4/2020 2:32 PM, Jerin Jacob wrote:  
> > > > On Mon, Aug 3, 2020 at 6:36 PM Slava Ovsiienko  
> > <viacheslavo@mellanox.com> wrote:  
> > > >>
> > > >> Hi, Jerin,
> > > >>
> > > >> Thanks for the comment,  please, see below.
> > > >>  
> > > >>> -----Original Message-----
> > > >>> From: Jerin Jacob <jerinjacobk@gmail.com>
> > > >>> Sent: Monday, August 3, 2020 14:57
> > > >>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > >>> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@mellanox.com>;
> > > >>> Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon
> > > >>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > > >>> Stephen Hemminger <stephen@networkplumber.org>; Andrew  
> > Rybchenko  
> > > >>> <arybchenko@solarflare.com>; Ajit Khaparde
> > > >>> <ajit.khaparde@broadcom.com>; Maxime Coquelin
> > > >>> <maxime.coquelin@redhat.com>; Olivier Matz
> > > >>> <olivier.matz@6wind.com>; David Marchand
> > > >>> <david.marchand@redhat.com>
> > > >>> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf
> > > >>> structure
> > > >>>
> > > >>> On Mon, Aug 3, 2020 at 4:28 PM Viacheslav Ovsiienko
> > > >>> <viacheslavo@mellanox.com> wrote:  
> > > >>>>
> > > >>>> The DPDK datapath in the transmit direction is very flexible.
> > > >>>> The applications can build multisegment packets and manages
> > > >>>> almost all data aspects - the memory pools where segments are
> > > >>>> allocated from, the segment lengths, the memory attributes like  
> > external, registered, etc.  
> > > >>>>
> > > >>>> In the receiving direction, the datapath is much less flexible,
> > > >>>> the applications can only specify the memory pool to configure
> > > >>>> the receiving queue and nothing more. In order to extend the
> > > >>>> receiving datapath capabilities it is proposed to add the new
> > > >>>> fields into rte_eth_rxconf structure:
> > > >>>>
> > > >>>> struct rte_eth_rxconf {
> > > >>>>     ...
> > > >>>>     uint16_t rx_split_num; /* number of segments to split */
> > > >>>>     uint16_t *rx_split_len; /* array of segment lengthes */
> > > >>>>     struct rte_mempool **mp; /* array of segment memory pools */  
> > > >>>
> > > >>> The pool has the packet length it's been configured for.
> > > >>> So I think, rx_split_len can be removed.  
> > > >>
> > > >> Yes, it is one of the supposed options - if pointer to array of
> > > >> segment lengths is NULL , the queue_setup() could use the lengths from  
> > the pool's properties.  
> > > >> But we are talking about packet split, in general, it should not
> > > >> depend on pool properties. What if application provides the single
> > > >> pool and just wants to have the tunnel header in the first dedicated  
> > mbuf?  
> > > >>  
> > > >>>
> > > >>> This feature also available in Marvell HW. So it not specific to one  
> > vendor.  
> > > >>> Maybe we could just the use case mention the use case in the
> > > >>> depreciation notice and the tentative change in rte_eth_rxconf and
> > > >>> exact details can be worked out at the time of implementation.
> > > >>>  
> > > >> So, if I understand correctly, the struct changes in the commit
> > > >> message should be marked as just possible implementation?  
> > > >
> > > > Yes.
> > > >
> > > > We may need to have a detailed discussion on the correct abstraction
> > > > for various HW is available with this feature.
> > > >
> > > > On Marvell HW, We can configure TWO pools for given eth Rx queue.
> > > > One pool can be configured as a small packet pool and other one as
> > > > large packet pool.
> > > > And there is a threshold value to decide the pool between small and  
> > large.  
> > > > For example:
> > > > - The small pool is configured 2k
> > > > - The large pool is configured with 10k
> > > > - And if the threshold value is configured as 2k.
> > > > Any packet size <=2K will land in small pool and others in a large pool.
> > > > The use case, we are targeting is to save the memory space for jumbo  
> > frames.  
> > >
> > > Out of curiosity, do you provide two different buffer address in the
> > > descriptor and HW automatically uses one based on the size, or driver
> > > uses one of the pools based on the configuration and possible largest
> > > packet size?  
> > 
> > I am all for allowing more configuration of buffer pool.
> > But don't want that to be exposed as a hardware specific requirement in the
> > API for applications. The worst case would be if your API changes required:
> > 
> >   if (strcmp(dev->driver_name, "marvell") == 0) {
> >      // make another mempool for this driver
> >   
> I thought about adding some other segment attributes, vendor specific.
> We could describe the segments with some descriptor structure (size, pool)
> and add flags field to one. The proposals from other vendors are welcome.
> 

Please no snowflake API's "are driver is special"...

Think of how it can fit into a general model.
Also, just because your hardware has a special feature does not mean
the DPDK has to support it!
  
Slava Ovsiienko Aug. 7, 2020, 11:23 a.m. UTC | #17
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, August 6, 2020 21:10
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Jerin Jacob
> <jerinjacobk@gmail.com>; dpdk-dev <dev@dpdk.org>; Matan Azrad
> <matan@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> Thomas Monjalon <thomas@monjalon.net>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Olivier Matz <olivier.matz@6wind.com>;
> David Marchand <david.marchand@redhat.com>
> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf structure
> 
> On Thu, 6 Aug 2020 17:03:31 +0000
> Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Thursday, August 6, 2020 19:26
> > > To: Ferruh Yigit <ferruh.yigit@intel.com>
> > > Cc: Jerin Jacob <jerinjacobk@gmail.com>; Slava Ovsiienko
> > > <viacheslavo@mellanox.com>; dpdk-dev <dev@dpdk.org>; Matan Azrad
> > > <matan@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> > > Thomas Monjalon <thomas@monjalon.net>; Andrew Rybchenko
> > > <arybchenko@solarflare.com>; Ajit Khaparde
> > > <ajit.khaparde@broadcom.com>; Maxime Coquelin
> > > <maxime.coquelin@redhat.com>; Olivier Matz
> <olivier.matz@6wind.com>;
> > > David Marchand <david.marchand@redhat.com>
> > > Subject: Re: [PATCH] doc: announce changes to ethdev rxconf
> > > structure
> > >
> > > On Thu, 6 Aug 2020 16:58:22 +0100
> > > Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > >
> > > > On 8/4/2020 2:32 PM, Jerin Jacob wrote:
> > > > > On Mon, Aug 3, 2020 at 6:36 PM Slava Ovsiienko
> > > <viacheslavo@mellanox.com> wrote:
> > > > >>
> > > > >> Hi, Jerin,
> > > > >>
> > > > >> Thanks for the comment,  please, see below.
> > > > >>
> > > > >>> -----Original Message-----
> > > > >>> From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > >>> Sent: Monday, August 3, 2020 14:57
> > > > >>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > > >>> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad
> <matan@mellanox.com>;
> > > > >>> Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon
> > > > >>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > > > >>> Stephen Hemminger <stephen@networkplumber.org>; Andrew
> > > Rybchenko
> > > > >>> <arybchenko@solarflare.com>; Ajit Khaparde
> > > > >>> <ajit.khaparde@broadcom.com>; Maxime Coquelin
> > > > >>> <maxime.coquelin@redhat.com>; Olivier Matz
> > > > >>> <olivier.matz@6wind.com>; David Marchand
> > > > >>> <david.marchand@redhat.com>
> > > > >>> Subject: Re: [PATCH] doc: announce changes to ethdev rxconf
> > > > >>> structure
> > > > >>>
> > > > >>> On Mon, Aug 3, 2020 at 4:28 PM Viacheslav Ovsiienko
> > > > >>> <viacheslavo@mellanox.com> wrote:
> > > > >>>>
> > > > >>>> The DPDK datapath in the transmit direction is very flexible.
> > > > >>>> The applications can build multisegment packets and manages
> > > > >>>> almost all data aspects - the memory pools where segments are
> > > > >>>> allocated from, the segment lengths, the memory attributes
> > > > >>>> like
> > > external, registered, etc.
> > > > >>>>
> > > > >>>> In the receiving direction, the datapath is much less
> > > > >>>> flexible, the applications can only specify the memory pool
> > > > >>>> to configure the receiving queue and nothing more. In order
> > > > >>>> to extend the receiving datapath capabilities it is proposed
> > > > >>>> to add the new fields into rte_eth_rxconf structure:
> > > > >>>>
> > > > >>>> struct rte_eth_rxconf {
> > > > >>>>     ...
> > > > >>>>     uint16_t rx_split_num; /* number of segments to split */
> > > > >>>>     uint16_t *rx_split_len; /* array of segment lengthes */
> > > > >>>>     struct rte_mempool **mp; /* array of segment memory pools
> > > > >>>> */
> > > > >>>
> > > > >>> The pool has the packet length it's been configured for.
> > > > >>> So I think, rx_split_len can be removed.
> > > > >>
> > > > >> Yes, it is one of the supposed options - if pointer to array of
> > > > >> segment lengths is NULL , the queue_setup() could use the
> > > > >> lengths from
> > > the pool's properties.
> > > > >> But we are talking about packet split, in general, it should
> > > > >> not depend on pool properties. What if application provides the
> > > > >> single pool and just wants to have the tunnel header in the
> > > > >> first dedicated
> > > mbuf?
> > > > >>
> > > > >>>
> > > > >>> This feature also available in Marvell HW. So it not specific
> > > > >>> to one
> > > vendor.
> > > > >>> Maybe we could just the use case mention the use case in the
> > > > >>> depreciation notice and the tentative change in rte_eth_rxconf
> > > > >>> and exact details can be worked out at the time of implementation.
> > > > >>>
> > > > >> So, if I understand correctly, the struct changes in the commit
> > > > >> message should be marked as just possible implementation?
> > > > >
> > > > > Yes.
> > > > >
> > > > > We may need to have a detailed discussion on the correct
> > > > > abstraction for various HW is available with this feature.
> > > > >
> > > > > On Marvell HW, We can configure TWO pools for given eth Rx queue.
> > > > > One pool can be configured as a small packet pool and other one
> > > > > as large packet pool.
> > > > > And there is a threshold value to decide the pool between small
> > > > > and
> > > large.
> > > > > For example:
> > > > > - The small pool is configured 2k
> > > > > - The large pool is configured with 10k
> > > > > - And if the threshold value is configured as 2k.
> > > > > Any packet size <=2K will land in small pool and others in a large pool.
> > > > > The use case, we are targeting is to save the memory space for
> > > > > jumbo
> > > frames.
> > > >
> > > > Out of curiosity, do you provide two different buffer address in
> > > > the descriptor and HW automatically uses one based on the size, or
> > > > driver uses one of the pools based on the configuration and
> > > > possible largest packet size?
> > >
> > > I am all for allowing more configuration of buffer pool.
> > > But don't want that to be exposed as a hardware specific requirement
> > > in the API for applications. The worst case would be if your API changes
> required:
> > >
> > >   if (strcmp(dev->driver_name, "marvell") == 0) {
> > >      // make another mempool for this driver
> > >
> > I thought about adding some other segment attributes, vendor specific.
> > We could describe the segments with some descriptor structure (size,
> > pool) and add flags field to one. The proposals from other vendors are
> welcome.
> >
> 
> Please no snowflake API's "are driver is special"...
> 
> Think of how it can fit into a general model.
> Also, just because your hardware has a special feature does not mean the
> DPDK has to support it!

Sure. The initial proposal is just about how to extend the Rx buffers description.
Now it is the single pool and the single fixed segment size only. Not very flexible so far.

With best regards, Slava
  

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ea4cfa7..cd700ae 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -99,6 +99,11 @@  Deprecation Notices
   In 19.11 PMDs will still update the field even when the offload is not
   enabled.
 
+* ethdev: add new fields to ``rte_eth_rxconf`` to configure the receiving
+  queues to split ingress packets into multiple segments according to the
+  specified lengths into the buffers allocated from the specified
+  memory pools. The backward compatibility to existing API is preserved.
+
 * ethdev: ``rx_descriptor_done`` dev_ops and ``rte_eth_rx_descriptor_done``
   will be deprecated in 20.11 and will be removed in 21.11.
   Existing ``rte_eth_rx_descriptor_status`` and ``rte_eth_tx_descriptor_status``