The maximum number of rings in af_packet is hard coded as 16. The user should be able to configure this as part of DPDK config. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- config/common_base | 1 + drivers/net/af_packet/rte_eth_af_packet.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/config/common_base b/config/common_base index 6ea9c63cc36b..dd154a474b57 100644 --- a/config/common_base +++ b/config/common_base @@ -468,6 +468,7 @@ CONFIG_RTE_LIBRTE_VMXNET3_DEBUG_TX_FREE=n # Compile software PMD backed by AF_PACKET sockets (Linux only) # CONFIG_RTE_LIBRTE_PMD_AF_PACKET=n +CONFIG_RTE_PMD_AF_PACKET_MAX_RINGS=16 # # Compile software PMD backed by AF_XDP sockets (Linux only) diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c index f5806bf42c46..6e90d231ca4f 100644 --- a/drivers/net/af_packet/rte_eth_af_packet.c +++ b/drivers/net/af_packet/rte_eth_af_packet.c @@ -6,6 +6,7 @@ * All rights reserved. */ +#include <rte_config.h> #include <rte_string_fns.h> #include <rte_mbuf.h> #include <rte_ethdev_driver.h> @@ -37,7 +38,9 @@ #define DFLT_FRAME_SIZE (1 << 11) #define DFLT_FRAME_COUNT (1 << 9) +#ifndef RTE_PMD_AF_PACKET_MAX_RINGS #define RTE_PMD_AF_PACKET_MAX_RINGS 16 +#endif struct pkt_rx_queue { int sockfd; -- 2.20.1
On 2/24/2020 11:11 PM, Stephen Hemminger wrote:
> The maximum number of rings in af_packet is hard coded as 16.
> The user should be able to configure this as part of DPDK config.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> config/common_base | 1 +
> drivers/net/af_packet/rte_eth_af_packet.c | 3 +++
> 2 files changed, 4 insertions(+)
>
> diff --git a/config/common_base b/config/common_base
> index 6ea9c63cc36b..dd154a474b57 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -468,6 +468,7 @@ CONFIG_RTE_LIBRTE_VMXNET3_DEBUG_TX_FREE=n
> # Compile software PMD backed by AF_PACKET sockets (Linux only)
> #
> CONFIG_RTE_LIBRTE_PMD_AF_PACKET=n
> +CONFIG_RTE_PMD_AF_PACKET_MAX_RINGS=16
>
Not sure about adding a new config option for this.
There is already "qpairs" devarg, allocating queues dynamically, instead of
using a per-defined sized arrays, should enable removing
'RTE_PMD_AF_PACKET_MAX_RINGS' completely.
On Thu, 27 Feb 2020 17:20:45 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 2/24/2020 11:11 PM, Stephen Hemminger wrote:
> > The maximum number of rings in af_packet is hard coded as 16.
> > The user should be able to configure this as part of DPDK config.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > config/common_base | 1 +
> > drivers/net/af_packet/rte_eth_af_packet.c | 3 +++
> > 2 files changed, 4 insertions(+)
> >
> > diff --git a/config/common_base b/config/common_base
> > index 6ea9c63cc36b..dd154a474b57 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -468,6 +468,7 @@ CONFIG_RTE_LIBRTE_VMXNET3_DEBUG_TX_FREE=n
> > # Compile software PMD backed by AF_PACKET sockets (Linux only)
> > #
> > CONFIG_RTE_LIBRTE_PMD_AF_PACKET=n
> > +CONFIG_RTE_PMD_AF_PACKET_MAX_RINGS=16
> >
>
> Not sure about adding a new config option for this.
>
> There is already "qpairs" devarg, allocating queues dynamically, instead of
> using a per-defined sized arrays, should enable removing
> 'RTE_PMD_AF_PACKET_MAX_RINGS' completely.
But the current driver does not allow qpairs > 16.
This is a config option to allow more.
On 2/27/2020 5:51 PM, Stephen Hemminger wrote:
> On Thu, 27 Feb 2020 17:20:45 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
>> On 2/24/2020 11:11 PM, Stephen Hemminger wrote:
>>> The maximum number of rings in af_packet is hard coded as 16.
>>> The user should be able to configure this as part of DPDK config.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>> config/common_base | 1 +
>>> drivers/net/af_packet/rte_eth_af_packet.c | 3 +++
>>> 2 files changed, 4 insertions(+)
>>>
>>> diff --git a/config/common_base b/config/common_base
>>> index 6ea9c63cc36b..dd154a474b57 100644
>>> --- a/config/common_base
>>> +++ b/config/common_base
>>> @@ -468,6 +468,7 @@ CONFIG_RTE_LIBRTE_VMXNET3_DEBUG_TX_FREE=n
>>> # Compile software PMD backed by AF_PACKET sockets (Linux only)
>>> #
>>> CONFIG_RTE_LIBRTE_PMD_AF_PACKET=n
>>> +CONFIG_RTE_PMD_AF_PACKET_MAX_RINGS=16
>>>
>>
>> Not sure about adding a new config option for this.
>>
>> There is already "qpairs" devarg, allocating queues dynamically, instead of
>> using a per-defined sized arrays, should enable removing
>> 'RTE_PMD_AF_PACKET_MAX_RINGS' completely.
>
> But the current driver does not allow qpairs > 16.
> This is a config option to allow more.
>
This can be done without config option by removing 16 limitation.
Since qpairs is part of the vdev arguments, there is no need to limit it to 16. The queue arrays can be dynamically sized based on the requested parameters. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- drivers/net/af_packet/rte_eth_af_packet.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c index f5806bf42c46..e5e0aa9277a8 100644 --- a/drivers/net/af_packet/rte_eth_af_packet.c +++ b/drivers/net/af_packet/rte_eth_af_packet.c @@ -37,8 +37,6 @@ #define DFLT_FRAME_SIZE (1 << 11) #define DFLT_FRAME_COUNT (1 << 9) -#define RTE_PMD_AF_PACKET_MAX_RINGS 16 - struct pkt_rx_queue { int sockfd; @@ -77,8 +75,8 @@ struct pmd_internals { struct tpacket_req req; - struct pkt_rx_queue rx_queue[RTE_PMD_AF_PACKET_MAX_RINGS]; - struct pkt_tx_queue tx_queue[RTE_PMD_AF_PACKET_MAX_RINGS]; + struct pkt_rx_queue *rx_queue; + struct pkt_tx_queue *tx_queue; }; static const char *valid_arguments[] = { @@ -601,6 +599,18 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, if (*internals == NULL) return -1; + + (*internals)->rx_queue = rte_calloc_socket("af_packet_rx", + nb_queues, + sizeof(struct pkt_rx_queue), + 0, numa_node); + (*internals)->tx_queue = rte_calloc_socket("af_packet_tx", + nb_queues, + sizeof(struct pkt_tx_queue), + 0, numa_node); + if (!(*internals)->rx_queue || !(*internals)->tx_queue) + return -1; + for (q = 0; q < nb_queues; q++) { (*internals)->rx_queue[q].map = MAP_FAILED; (*internals)->tx_queue[q].map = MAP_FAILED; @@ -846,8 +856,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev, pair = &kvlist->pairs[k_idx]; if (strstr(pair->key, ETH_AF_PACKET_NUM_Q_ARG) != NULL) { qpairs = atoi(pair->value); - if (qpairs < 1 || - qpairs > RTE_PMD_AF_PACKET_MAX_RINGS) { + if (qpairs < 1) { PMD_LOG(ERR, "%s: invalid qpairs value", name); @@ -1019,6 +1028,8 @@ rte_pmd_af_packet_remove(struct rte_vdev_device *dev) rte_free(internals->tx_queue[q].rd); } free(internals->if_name); + rte_free(internals->rx_queue); + rte_free(internals->tx_queue); rte_eth_dev_release_port(eth_dev); -- 2.20.1
On Thu, Feb 27, 2020 at 12:00:03PM -0800, Stephen Hemminger wrote: > Since qpairs is part of the vdev arguments, there is no need to > limit it to 16. The queue arrays can be dynamically sized based > on the requested parameters. > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> LGTM! Acked-by: John W. Linville <linville@tuxdriver.com> > --- > drivers/net/af_packet/rte_eth_af_packet.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c > index f5806bf42c46..e5e0aa9277a8 100644 > --- a/drivers/net/af_packet/rte_eth_af_packet.c > +++ b/drivers/net/af_packet/rte_eth_af_packet.c > @@ -37,8 +37,6 @@ > #define DFLT_FRAME_SIZE (1 << 11) > #define DFLT_FRAME_COUNT (1 << 9) > > -#define RTE_PMD_AF_PACKET_MAX_RINGS 16 > - > struct pkt_rx_queue { > int sockfd; > > @@ -77,8 +75,8 @@ struct pmd_internals { > > struct tpacket_req req; > > - struct pkt_rx_queue rx_queue[RTE_PMD_AF_PACKET_MAX_RINGS]; > - struct pkt_tx_queue tx_queue[RTE_PMD_AF_PACKET_MAX_RINGS]; > + struct pkt_rx_queue *rx_queue; > + struct pkt_tx_queue *tx_queue; > }; > > static const char *valid_arguments[] = { > @@ -601,6 +599,18 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, > if (*internals == NULL) > return -1; > > + > + (*internals)->rx_queue = rte_calloc_socket("af_packet_rx", > + nb_queues, > + sizeof(struct pkt_rx_queue), > + 0, numa_node); > + (*internals)->tx_queue = rte_calloc_socket("af_packet_tx", > + nb_queues, > + sizeof(struct pkt_tx_queue), > + 0, numa_node); > + if (!(*internals)->rx_queue || !(*internals)->tx_queue) > + return -1; > + > for (q = 0; q < nb_queues; q++) { > (*internals)->rx_queue[q].map = MAP_FAILED; > (*internals)->tx_queue[q].map = MAP_FAILED; > @@ -846,8 +856,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev, > pair = &kvlist->pairs[k_idx]; > if (strstr(pair->key, ETH_AF_PACKET_NUM_Q_ARG) != NULL) { > qpairs = atoi(pair->value); > - if (qpairs < 1 || > - qpairs > RTE_PMD_AF_PACKET_MAX_RINGS) { > + if (qpairs < 1) { > PMD_LOG(ERR, > "%s: invalid qpairs value", > name); > @@ -1019,6 +1028,8 @@ rte_pmd_af_packet_remove(struct rte_vdev_device *dev) > rte_free(internals->tx_queue[q].rd); > } > free(internals->if_name); > + rte_free(internals->rx_queue); > + rte_free(internals->tx_queue); > > rte_eth_dev_release_port(eth_dev); > > -- > 2.20.1 > > -- John W. Linville Someday the world will need a hero, and you linville@tuxdriver.com might be all we have. Be ready.
On 2/27/2020 8:00 PM, Stephen Hemminger wrote: > Since qpairs is part of the vdev arguments, there is no need to > limit it to 16. The queue arrays can be dynamically sized based > on the requested parameters. > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > drivers/net/af_packet/rte_eth_af_packet.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c > index f5806bf42c46..e5e0aa9277a8 100644 > --- a/drivers/net/af_packet/rte_eth_af_packet.c > +++ b/drivers/net/af_packet/rte_eth_af_packet.c > @@ -37,8 +37,6 @@ > #define DFLT_FRAME_SIZE (1 << 11) > #define DFLT_FRAME_COUNT (1 << 9) > > -#define RTE_PMD_AF_PACKET_MAX_RINGS 16 > - > struct pkt_rx_queue { > int sockfd; > > @@ -77,8 +75,8 @@ struct pmd_internals { > > struct tpacket_req req; > > - struct pkt_rx_queue rx_queue[RTE_PMD_AF_PACKET_MAX_RINGS]; > - struct pkt_tx_queue tx_queue[RTE_PMD_AF_PACKET_MAX_RINGS]; > + struct pkt_rx_queue *rx_queue; > + struct pkt_tx_queue *tx_queue; > }; > > static const char *valid_arguments[] = { > @@ -601,6 +599,18 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, > if (*internals == NULL) > return -1; > > + > + (*internals)->rx_queue = rte_calloc_socket("af_packet_rx", > + nb_queues, > + sizeof(struct pkt_rx_queue), > + 0, numa_node); > + (*internals)->tx_queue = rte_calloc_socket("af_packet_tx", > + nb_queues, > + sizeof(struct pkt_tx_queue), > + 0, numa_node); Not for this patch but right now all queue initialization done during init based on max queue PMD can support, we may move allocating and configuring queues in 'eth_rx_queue_setup' & 'eth_tx_queue_setup' based on number of queue application request, in the future... > + if (!(*internals)->rx_queue || !(*internals)->tx_queue) > + return -1; If only one allocation fails, should we free the other? > + > for (q = 0; q < nb_queues; q++) { > (*internals)->rx_queue[q].map = MAP_FAILED; > (*internals)->tx_queue[q].map = MAP_FAILED; > @@ -846,8 +856,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev, > pair = &kvlist->pairs[k_idx]; > if (strstr(pair->key, ETH_AF_PACKET_NUM_Q_ARG) != NULL) { > qpairs = atoi(pair->value); > - if (qpairs < 1 || > - qpairs > RTE_PMD_AF_PACKET_MAX_RINGS) { > + if (qpairs < 1) { > PMD_LOG(ERR, > "%s: invalid qpairs value", > name); > @@ -1019,6 +1028,8 @@ rte_pmd_af_packet_remove(struct rte_vdev_device *dev) > rte_free(internals->tx_queue[q].rd); > } > free(internals->if_name); > + rte_free(internals->rx_queue); > + rte_free(internals->tx_queue); > > rte_eth_dev_release_port(eth_dev); > >
On Fri, Feb 28, 2020 at 10:08:43AM +0000, Ferruh Yigit wrote: > On 2/27/2020 8:00 PM, Stephen Hemminger wrote: > > Since qpairs is part of the vdev arguments, there is no need to > > limit it to 16. The queue arrays can be dynamically sized based > > on the requested parameters. > > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > > --- > > drivers/net/af_packet/rte_eth_af_packet.c | 23 +++++++++++++++++------ > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c > > index f5806bf42c46..e5e0aa9277a8 100644 > > --- a/drivers/net/af_packet/rte_eth_af_packet.c > > +++ b/drivers/net/af_packet/rte_eth_af_packet.c > > @@ -37,8 +37,6 @@ > > #define DFLT_FRAME_SIZE (1 << 11) > > #define DFLT_FRAME_COUNT (1 << 9) > > > > -#define RTE_PMD_AF_PACKET_MAX_RINGS 16 > > - > > struct pkt_rx_queue { > > int sockfd; > > > > @@ -77,8 +75,8 @@ struct pmd_internals { > > > > struct tpacket_req req; > > > > - struct pkt_rx_queue rx_queue[RTE_PMD_AF_PACKET_MAX_RINGS]; > > - struct pkt_tx_queue tx_queue[RTE_PMD_AF_PACKET_MAX_RINGS]; > > + struct pkt_rx_queue *rx_queue; > > + struct pkt_tx_queue *tx_queue; > > }; > > > > static const char *valid_arguments[] = { > > @@ -601,6 +599,18 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, > > if (*internals == NULL) > > return -1; > > > > + > > + (*internals)->rx_queue = rte_calloc_socket("af_packet_rx", > > + nb_queues, > > + sizeof(struct pkt_rx_queue), > > + 0, numa_node); > > + (*internals)->tx_queue = rte_calloc_socket("af_packet_tx", > > + nb_queues, > > + sizeof(struct pkt_tx_queue), > > + 0, numa_node); > > Not for this patch but right now all queue initialization done during init based > on max queue PMD can support, we may move allocating and configuring queues in > 'eth_rx_queue_setup' & 'eth_tx_queue_setup' based on number of queue application > request, in the future... > > > + if (!(*internals)->rx_queue || !(*internals)->tx_queue) > > + return -1; > > If only one allocation fails, should we free the other? Yeah, good catch. > > + > > for (q = 0; q < nb_queues; q++) { > > (*internals)->rx_queue[q].map = MAP_FAILED; > > (*internals)->tx_queue[q].map = MAP_FAILED; > > @@ -846,8 +856,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev, > > pair = &kvlist->pairs[k_idx]; > > if (strstr(pair->key, ETH_AF_PACKET_NUM_Q_ARG) != NULL) { > > qpairs = atoi(pair->value); > > - if (qpairs < 1 || > > - qpairs > RTE_PMD_AF_PACKET_MAX_RINGS) { > > + if (qpairs < 1) { > > PMD_LOG(ERR, > > "%s: invalid qpairs value", > > name); > > @@ -1019,6 +1028,8 @@ rte_pmd_af_packet_remove(struct rte_vdev_device *dev) > > rte_free(internals->tx_queue[q].rd); > > } > > free(internals->if_name); > > + rte_free(internals->rx_queue); > > + rte_free(internals->tx_queue); > > > > rte_eth_dev_release_port(eth_dev); > > > > > > -- John W. Linville Someday the world will need a hero, and you linville@tuxdriver.com might be all we have. Be ready.
On 2/28/2020 4:52 PM, John W. Linville wrote:
> On Fri, Feb 28, 2020 at 10:08:43AM +0000, Ferruh Yigit wrote:
>> On 2/27/2020 8:00 PM, Stephen Hemminger wrote:
>>> Since qpairs is part of the vdev arguments, there is no need to
>>> limit it to 16. The queue arrays can be dynamically sized based
>>> on the requested parameters.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>> drivers/net/af_packet/rte_eth_af_packet.c | 23 +++++++++++++++++------
>>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
>>> index f5806bf42c46..e5e0aa9277a8 100644
>>> --- a/drivers/net/af_packet/rte_eth_af_packet.c
>>> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
>>> @@ -37,8 +37,6 @@
>>> #define DFLT_FRAME_SIZE (1 << 11)
>>> #define DFLT_FRAME_COUNT (1 << 9)
>>>
>>> -#define RTE_PMD_AF_PACKET_MAX_RINGS 16
>>> -
>>> struct pkt_rx_queue {
>>> int sockfd;
>>>
>>> @@ -77,8 +75,8 @@ struct pmd_internals {
>>>
>>> struct tpacket_req req;
>>>
>>> - struct pkt_rx_queue rx_queue[RTE_PMD_AF_PACKET_MAX_RINGS];
>>> - struct pkt_tx_queue tx_queue[RTE_PMD_AF_PACKET_MAX_RINGS];
>>> + struct pkt_rx_queue *rx_queue;
>>> + struct pkt_tx_queue *tx_queue;
>>> };
>>>
>>> static const char *valid_arguments[] = {
>>> @@ -601,6 +599,18 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>>> if (*internals == NULL)
>>> return -1;
>>>
>>> +
>>> + (*internals)->rx_queue = rte_calloc_socket("af_packet_rx",
>>> + nb_queues,
>>> + sizeof(struct pkt_rx_queue),
>>> + 0, numa_node);
>>> + (*internals)->tx_queue = rte_calloc_socket("af_packet_tx",
>>> + nb_queues,
>>> + sizeof(struct pkt_tx_queue),
>>> + 0, numa_node);
>>
>> Not for this patch but right now all queue initialization done during init based
>> on max queue PMD can support, we may move allocating and configuring queues in
>> 'eth_rx_queue_setup' & 'eth_tx_queue_setup' based on number of queue application
>> request, in the future...
>>
>>> + if (!(*internals)->rx_queue || !(*internals)->tx_queue)
>>> + return -1;
>>
>> If only one allocation fails, should we free the other?
>
> Yeah, good catch.
>
Will fix while merging.
On 2/27/2020 8:56 PM, John W. Linville wrote:
> On Thu, Feb 27, 2020 at 12:00:03PM -0800, Stephen Hemminger wrote:
>> Since qpairs is part of the vdev arguments, there is no need to
>> limit it to 16. The queue arrays can be dynamically sized based
>> on the requested parameters.
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>
> LGTM!
>
> Acked-by: John W. Linville <linville@tuxdriver.com>
>
Applied to dpdk-next-net/master, thanks.
(Mentioned possible failure path memory leak fixed while merging)
On Mon, 2 Mar 2020 16:24:23 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 2/27/2020 8:56 PM, John W. Linville wrote:
> > On Thu, Feb 27, 2020 at 12:00:03PM -0800, Stephen Hemminger wrote:
> >> Since qpairs is part of the vdev arguments, there is no need to
> >> limit it to 16. The queue arrays can be dynamically sized based
> >> on the requested parameters.
> >>
> >> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >
> > LGTM!
> >
> > Acked-by: John W. Linville <linville@tuxdriver.com>
> >
>
> Applied to dpdk-next-net/master, thanks.
>
> (Mentioned possible failure path memory leak fixed while merging)
The driver also needs updated close logic.
On 3/2/2020 4:43 PM, Stephen Hemminger wrote:
> On Mon, 2 Mar 2020 16:24:23 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
>> On 2/27/2020 8:56 PM, John W. Linville wrote:
>>> On Thu, Feb 27, 2020 at 12:00:03PM -0800, Stephen Hemminger wrote:
>>>> Since qpairs is part of the vdev arguments, there is no need to
>>>> limit it to 16. The queue arrays can be dynamically sized based
>>>> on the requested parameters.
>>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>
>>> LGTM!
>>>
>>> Acked-by: John W. Linville <linville@tuxdriver.com>
>>>
>>
>> Applied to dpdk-next-net/master, thanks.
>>
>> (Mentioned possible failure path memory leak fixed while merging)
>
> The driver also needs updated close logic.
>
+1