diff mbox

[dpdk-dev] net/vmxnet3: fix queue size changes

Message ID	1489444915-3660-1-git-send-email-ciwillia@brocade.com (mailing list archive)
State	Superseded, archived
Delegated to:	Ferruh Yigit
Headers	From: "Charles (Chas) Williams" <ciwillia@brocade.com> To: <dev@dpdk.org> CC: <yongwang@vmware.com>, "Charles (Chas) Williams" <ciwillia@brocade.com> Date: Mon, 13 Mar 2017 18:41:55 -0400 Message-ID: <1489444915-3660-1-git-send-email-ciwillia@brocade.com> MIME-Version: 1.0 Content-Type: text/plain Subject: [dpdk-dev] [PATCH] net/vmxnet3: fix queue size changes Precedence: list Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org>

Checks

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

Commit Message

Chas Williams March 13, 2017, 10:41 p.m. UTC

  If the user reconfigures the queues size, then the previosly allocated
memzone may potentially be too small.  Instead, always free the old
memzone and allocate a new one.

Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")

Signed-off-by: Chas Williams <ciwillia@brocade.com>
---
 drivers/net/vmxnet3/vmxnet3_rxtx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Jan Blunck March 14, 2017, 4:11 p.m. UTC | #1

On Mon, Mar 13, 2017 at 11:41 PM, Charles (Chas) Williams
<ciwillia@brocade.com> wrote:
> If the user reconfigures the queues size, then the previosly allocated
> memzone may potentially be too small.  Instead, always free the old
> memzone and allocate a new one.
>
> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
>
> Signed-off-by: Chas Williams <ciwillia@brocade.com>
> ---
>  drivers/net/vmxnet3/vmxnet3_rxtx.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> index 6649c3f..104e040 100644
> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> @@ -893,8 +893,8 @@ vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
>
>  /*
>   * Create memzone for device rings. malloc can't be used as the physical address is
> - * needed. If the memzone is already created, then this function returns a ptr
> - * to the old one.
> + * needed. If the memzone already exists, we free it since it may have been created
> + * with a different size.
>   */
>  static const struct rte_memzone *
>  ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
> @@ -909,7 +909,7 @@ ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
>
>         mz = rte_memzone_lookup(z_name);
>         if (mz)
> -               return mz;
> +               rte_memzone_free(mz);
>
>         return rte_memzone_reserve_aligned(z_name, ring_size,
>                                            socket_id, 0, VMXNET3_RING_BA_ALIGN);

Chas,

Thanks for hunting this one down. Wouldn't the rte_memzone_free()
better fit into vmxnet3_cmd_ring_release() ?

Also the ring_dma_zone_reserve() could get replaced by
rte_eth_dma_zone_reserve() (see also
http://dpdk.org/dev/patchwork/patch/21432/).

Chas Williams March 14, 2017, 4:38 p.m. UTC | #2

On 03/14/2017 12:11 PM, Jan Blunck wrote:
> On Mon, Mar 13, 2017 at 11:41 PM, Charles (Chas) Williams
> <ciwillia@brocade.com> wrote:
>> If the user reconfigures the queues size, then the previosly allocated
>> memzone may potentially be too small.  Instead, always free the old
>> memzone and allocate a new one.
>>
>> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
>>
>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>> ---
>>  drivers/net/vmxnet3/vmxnet3_rxtx.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>> index 6649c3f..104e040 100644
>> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>> @@ -893,8 +893,8 @@ vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
>>
>>  /*
>>   * Create memzone for device rings. malloc can't be used as the physical address is
>> - * needed. If the memzone is already created, then this function returns a ptr
>> - * to the old one.
>> + * needed. If the memzone already exists, we free it since it may have been created
>> + * with a different size.
>>   */
>>  static const struct rte_memzone *
>>  ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
>> @@ -909,7 +909,7 @@ ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
>>
>>         mz = rte_memzone_lookup(z_name);
>>         if (mz)
>> -               return mz;
>> +               rte_memzone_free(mz);
>>
>>         return rte_memzone_reserve_aligned(z_name, ring_size,
>>                                            socket_id, 0, VMXNET3_RING_BA_ALIGN);
>
> Chas,
>
> Thanks for hunting this one down. Wouldn't the rte_memzone_free()
> better fit into vmxnet3_cmd_ring_release() ?

I don't care which way it goes.  I just did what is basically done in
gpa_zone_reserve() to match the "style".  Tracking the current ring size
and avoiding reallocating a potentially large chunk of memory seems like
a better idea.

> Also the ring_dma_zone_reserve() could get replaced by
> rte_eth_dma_zone_reserve() (see also

Yes, it probably should get changed to that along with tracking the size.

Jan Blunck March 15, 2017, 8:18 a.m. UTC | #3

On Tue, Mar 14, 2017 at 5:38 PM, Charles (Chas) Williams
<ciwillia@brocade.com> wrote:
>
>
> On 03/14/2017 12:11 PM, Jan Blunck wrote:
>>
>> On Mon, Mar 13, 2017 at 11:41 PM, Charles (Chas) Williams
>> <ciwillia@brocade.com> wrote:
>>>
>>> If the user reconfigures the queues size, then the previosly allocated
>>> memzone may potentially be too small.  Instead, always free the old
>>> memzone and allocate a new one.
>>>
>>> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver
>>> implementation")
>>>
>>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>>> ---
>>>  drivers/net/vmxnet3/vmxnet3_rxtx.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>> b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>> index 6649c3f..104e040 100644
>>> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>> @@ -893,8 +893,8 @@ vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf
>>> **rx_pkts, uint16_t nb_pkts)
>>>
>>>  /*
>>>   * Create memzone for device rings. malloc can't be used as the physical
>>> address is
>>> - * needed. If the memzone is already created, then this function returns
>>> a ptr
>>> - * to the old one.
>>> + * needed. If the memzone already exists, we free it since it may have
>>> been created
>>> + * with a different size.
>>>   */
>>>  static const struct rte_memzone *
>>>  ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
>>> @@ -909,7 +909,7 @@ ring_dma_zone_reserve(struct rte_eth_dev *dev, const
>>> char *ring_name,
>>>
>>>         mz = rte_memzone_lookup(z_name);
>>>         if (mz)
>>> -               return mz;
>>> +               rte_memzone_free(mz);
>>>
>>>         return rte_memzone_reserve_aligned(z_name, ring_size,
>>>                                            socket_id, 0,
>>> VMXNET3_RING_BA_ALIGN);
>>
>>
>> Chas,
>>
>> Thanks for hunting this one down. Wouldn't the rte_memzone_free()
>> better fit into vmxnet3_cmd_ring_release() ?
>
>
> I don't care which way it goes.  I just did what is basically done in
> gpa_zone_reserve() to match the "style".  Tracking the current ring size
> and avoiding reallocating a potentially large chunk of memory seems like
> a better idea.
>
>> Also the ring_dma_zone_reserve() could get replaced by
>> rte_eth_dma_zone_reserve() (see also
>
>
> Yes, it probably should get changed to that along with tracking the size.

Why don't we always allocate VMXNET3_RX_RING_MAX_SIZE entries? That
way we don't need to reallocate on a later queue setup change?

Chas Williams March 15, 2017, 9:45 a.m. UTC | #4

On 03/15/2017 04:18 AM, Jan Blunck wrote:
> On Tue, Mar 14, 2017 at 5:38 PM, Charles (Chas) Williams
> <ciwillia@brocade.com> wrote:
>>
>>
>> On 03/14/2017 12:11 PM, Jan Blunck wrote:
>>>
>>> On Mon, Mar 13, 2017 at 11:41 PM, Charles (Chas) Williams
>>> <ciwillia@brocade.com> wrote:
>>>>
>>>> If the user reconfigures the queues size, then the previosly allocated
>>>> memzone may potentially be too small.  Instead, always free the old
>>>> memzone and allocate a new one.
>>>>
>>>> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver
>>>> implementation")
>>>>
>>>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>>>> ---
>>>>  drivers/net/vmxnet3/vmxnet3_rxtx.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>> b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>> index 6649c3f..104e040 100644
>>>> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>> @@ -893,8 +893,8 @@ vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf
>>>> **rx_pkts, uint16_t nb_pkts)
>>>>
>>>>  /*
>>>>   * Create memzone for device rings. malloc can't be used as the physical
>>>> address is
>>>> - * needed. If the memzone is already created, then this function returns
>>>> a ptr
>>>> - * to the old one.
>>>> + * needed. If the memzone already exists, we free it since it may have
>>>> been created
>>>> + * with a different size.
>>>>   */
>>>>  static const struct rte_memzone *
>>>>  ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
>>>> @@ -909,7 +909,7 @@ ring_dma_zone_reserve(struct rte_eth_dev *dev, const
>>>> char *ring_name,
>>>>
>>>>         mz = rte_memzone_lookup(z_name);
>>>>         if (mz)
>>>> -               return mz;
>>>> +               rte_memzone_free(mz);
>>>>
>>>>         return rte_memzone_reserve_aligned(z_name, ring_size,
>>>>                                            socket_id, 0,
>>>> VMXNET3_RING_BA_ALIGN);
>>>
>>>
>>> Chas,
>>>
>>> Thanks for hunting this one down. Wouldn't the rte_memzone_free()
>>> better fit into vmxnet3_cmd_ring_release() ?
>>
>>
>> I don't care which way it goes.  I just did what is basically done in
>> gpa_zone_reserve() to match the "style".  Tracking the current ring size
>> and avoiding reallocating a potentially large chunk of memory seems like
>> a better idea.
>>
>>> Also the ring_dma_zone_reserve() could get replaced by
>>> rte_eth_dma_zone_reserve() (see also
>>
>>
>> Yes, it probably should get changed to that along with tracking the size.
>
> Why don't we always allocate VMXNET3_RX_RING_MAX_SIZE entries? That
> way we don't need to reallocate on a later queue setup change?

That's using more memory than it needs to use.  It might break someone's application
that already runs in some tightly constrained machine.  Failing to shrink the memzone
isn't likely to break anything since they have (apparently) already dealt with having
less memory available before switching to a smaller queue size.

Still, it can be a matter for another day.

Jan Blunck March 15, 2017, 10:05 a.m. UTC | #5

On Wed, Mar 15, 2017 at 10:45 AM, Charles (Chas) Williams
<ciwillia@brocade.com> wrote:
>
>
> On 03/15/2017 04:18 AM, Jan Blunck wrote:
>>
>> On Tue, Mar 14, 2017 at 5:38 PM, Charles (Chas) Williams
>> <ciwillia@brocade.com> wrote:
>>>
>>>
>>>
>>> On 03/14/2017 12:11 PM, Jan Blunck wrote:
>>>>
>>>>
>>>> On Mon, Mar 13, 2017 at 11:41 PM, Charles (Chas) Williams
>>>> <ciwillia@brocade.com> wrote:
>>>>>
>>>>>
>>>>> If the user reconfigures the queues size, then the previosly allocated
>>>>> memzone may potentially be too small.  Instead, always free the old
>>>>> memzone and allocate a new one.
>>>>>
>>>>> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver
>>>>> implementation")
>>>>>
>>>>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>>>>> ---
>>>>>  drivers/net/vmxnet3/vmxnet3_rxtx.c | 6 +++---
>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>> b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>> index 6649c3f..104e040 100644
>>>>> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>> @@ -893,8 +893,8 @@ vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf
>>>>> **rx_pkts, uint16_t nb_pkts)
>>>>>
>>>>>  /*
>>>>>   * Create memzone for device rings. malloc can't be used as the
>>>>> physical
>>>>> address is
>>>>> - * needed. If the memzone is already created, then this function
>>>>> returns
>>>>> a ptr
>>>>> - * to the old one.
>>>>> + * needed. If the memzone already exists, we free it since it may have
>>>>> been created
>>>>> + * with a different size.
>>>>>   */
>>>>>  static const struct rte_memzone *
>>>>>  ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
>>>>> @@ -909,7 +909,7 @@ ring_dma_zone_reserve(struct rte_eth_dev *dev,
>>>>> const
>>>>> char *ring_name,
>>>>>
>>>>>         mz = rte_memzone_lookup(z_name);
>>>>>         if (mz)
>>>>> -               return mz;
>>>>> +               rte_memzone_free(mz);
>>>>>
>>>>>         return rte_memzone_reserve_aligned(z_name, ring_size,
>>>>>                                            socket_id, 0,
>>>>> VMXNET3_RING_BA_ALIGN);
>>>>
>>>>
>>>>
>>>> Chas,
>>>>
>>>> Thanks for hunting this one down. Wouldn't the rte_memzone_free()
>>>> better fit into vmxnet3_cmd_ring_release() ?
>>>
>>>
>>>
>>> I don't care which way it goes.  I just did what is basically done in
>>> gpa_zone_reserve() to match the "style".  Tracking the current ring size
>>> and avoiding reallocating a potentially large chunk of memory seems like
>>> a better idea.
>>>
>>>> Also the ring_dma_zone_reserve() could get replaced by
>>>> rte_eth_dma_zone_reserve() (see also
>>>
>>>
>>>
>>> Yes, it probably should get changed to that along with tracking the size.
>>
>>
>> Why don't we always allocate VMXNET3_RX_RING_MAX_SIZE entries? That
>> way we don't need to reallocate on a later queue setup change?
>
>
> That's using more memory than it needs to use.  It might break someone's
> application
> that already runs in some tightly constrained machine.  Failing to shrink
> the memzone
> isn't likely to break anything since they have (apparently) already dealt
> with having
> less memory available before switching to a smaller queue size.
>
> Still, it can be a matter for another day.
>

Other drivers (ixgbe, e1000, ...) always allocate based on the max
ring size too. Since the VMXNET3_RX_RING_MAX_SIZE is 4096 I don't
think it is a huge waste of memory.

Chas Williams March 15, 2017, 10:06 a.m. UTC | #6

On 03/15/2017 06:05 AM, Jan Blunck wrote:
> On Wed, Mar 15, 2017 at 10:45 AM, Charles (Chas) Williams
> <ciwillia@brocade.com> wrote:
>>
>>
>> On 03/15/2017 04:18 AM, Jan Blunck wrote:
>>>
>>> On Tue, Mar 14, 2017 at 5:38 PM, Charles (Chas) Williams
>>> <ciwillia@brocade.com> wrote:
>>>>
>>>>
>>>>
>>>> On 03/14/2017 12:11 PM, Jan Blunck wrote:
>>>>>
>>>>>
>>>>> On Mon, Mar 13, 2017 at 11:41 PM, Charles (Chas) Williams
>>>>> <ciwillia@brocade.com> wrote:
>>>>>>
>>>>>>
>>>>>> If the user reconfigures the queues size, then the previosly allocated
>>>>>> memzone may potentially be too small.  Instead, always free the old
>>>>>> memzone and allocate a new one.
>>>>>>
>>>>>> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver
>>>>>> implementation")
>>>>>>
>>>>>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>>>>>> ---
>>>>>>  drivers/net/vmxnet3/vmxnet3_rxtx.c | 6 +++---
>>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>>> b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>>> index 6649c3f..104e040 100644
>>>>>> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>>> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>>> @@ -893,8 +893,8 @@ vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf
>>>>>> **rx_pkts, uint16_t nb_pkts)
>>>>>>
>>>>>>  /*
>>>>>>   * Create memzone for device rings. malloc can't be used as the
>>>>>> physical
>>>>>> address is
>>>>>> - * needed. If the memzone is already created, then this function
>>>>>> returns
>>>>>> a ptr
>>>>>> - * to the old one.
>>>>>> + * needed. If the memzone already exists, we free it since it may have
>>>>>> been created
>>>>>> + * with a different size.
>>>>>>   */
>>>>>>  static const struct rte_memzone *
>>>>>>  ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
>>>>>> @@ -909,7 +909,7 @@ ring_dma_zone_reserve(struct rte_eth_dev *dev,
>>>>>> const
>>>>>> char *ring_name,
>>>>>>
>>>>>>         mz = rte_memzone_lookup(z_name);
>>>>>>         if (mz)
>>>>>> -               return mz;
>>>>>> +               rte_memzone_free(mz);
>>>>>>
>>>>>>         return rte_memzone_reserve_aligned(z_name, ring_size,
>>>>>>                                            socket_id, 0,
>>>>>> VMXNET3_RING_BA_ALIGN);
>>>>>
>>>>>
>>>>>
>>>>> Chas,
>>>>>
>>>>> Thanks for hunting this one down. Wouldn't the rte_memzone_free()
>>>>> better fit into vmxnet3_cmd_ring_release() ?
>>>>
>>>>
>>>>
>>>> I don't care which way it goes.  I just did what is basically done in
>>>> gpa_zone_reserve() to match the "style".  Tracking the current ring size
>>>> and avoiding reallocating a potentially large chunk of memory seems like
>>>> a better idea.
>>>>
>>>>> Also the ring_dma_zone_reserve() could get replaced by
>>>>> rte_eth_dma_zone_reserve() (see also
>>>>
>>>>
>>>>
>>>> Yes, it probably should get changed to that along with tracking the size.
>>>
>>>
>>> Why don't we always allocate VMXNET3_RX_RING_MAX_SIZE entries? That
>>> way we don't need to reallocate on a later queue setup change?
>>
>>
>> That's using more memory than it needs to use.  It might break someone's
>> application
>> that already runs in some tightly constrained machine.  Failing to shrink
>> the memzone
>> isn't likely to break anything since they have (apparently) already dealt
>> with having
>> less memory available before switching to a smaller queue size.
>>
>> Still, it can be a matter for another day.
>>
>
> Other drivers (ixgbe, e1000, ...) always allocate based on the max
> ring size too. Since the VMXNET3_RX_RING_MAX_SIZE is 4096 I don't
> think it is a huge waste of memory.

OK.  BTW, the RX queues have the same issue.

Chas Williams March 15, 2017, 12:34 p.m. UTC | #7

On 03/15/2017 06:05 AM, Jan Blunck wrote:
> On Wed, Mar 15, 2017 at 10:45 AM, Charles (Chas) Williams
> <ciwillia@brocade.com> wrote:
>>
>>
>> On 03/15/2017 04:18 AM, Jan Blunck wrote:
>>>
>>> On Tue, Mar 14, 2017 at 5:38 PM, Charles (Chas) Williams
>>> <ciwillia@brocade.com> wrote:
>>>>
>>>>
>>>>
>>>> On 03/14/2017 12:11 PM, Jan Blunck wrote:
>>>>>
>>>>>
>>>>> On Mon, Mar 13, 2017 at 11:41 PM, Charles (Chas) Williams
>>>>> <ciwillia@brocade.com> wrote:
>>>>>>
>>>>>>
>>>>>> If the user reconfigures the queues size, then the previosly allocated
>>>>>> memzone may potentially be too small.  Instead, always free the old
>>>>>> memzone and allocate a new one.
>>>>>>
>>>>>> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver
>>>>>> implementation")
>>>>>>
>>>>>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>>>>>> ---
>>>>>>  drivers/net/vmxnet3/vmxnet3_rxtx.c | 6 +++---
>>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>>> b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>>> index 6649c3f..104e040 100644
>>>>>> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>>> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>>>> @@ -893,8 +893,8 @@ vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf
>>>>>> **rx_pkts, uint16_t nb_pkts)
>>>>>>
>>>>>>  /*
>>>>>>   * Create memzone for device rings. malloc can't be used as the
>>>>>> physical
>>>>>> address is
>>>>>> - * needed. If the memzone is already created, then this function
>>>>>> returns
>>>>>> a ptr
>>>>>> - * to the old one.
>>>>>> + * needed. If the memzone already exists, we free it since it may have
>>>>>> been created
>>>>>> + * with a different size.
>>>>>>   */
>>>>>>  static const struct rte_memzone *
>>>>>>  ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
>>>>>> @@ -909,7 +909,7 @@ ring_dma_zone_reserve(struct rte_eth_dev *dev,
>>>>>> const
>>>>>> char *ring_name,
>>>>>>
>>>>>>         mz = rte_memzone_lookup(z_name);
>>>>>>         if (mz)
>>>>>> -               return mz;
>>>>>> +               rte_memzone_free(mz);
>>>>>>
>>>>>>         return rte_memzone_reserve_aligned(z_name, ring_size,
>>>>>>                                            socket_id, 0,
>>>>>> VMXNET3_RING_BA_ALIGN);
>>>>>
>>>>>
>>>>>
>>>>> Chas,
>>>>>
>>>>> Thanks for hunting this one down. Wouldn't the rte_memzone_free()
>>>>> better fit into vmxnet3_cmd_ring_release() ?
>>>>
>>>>
>>>>
>>>> I don't care which way it goes.  I just did what is basically done in
>>>> gpa_zone_reserve() to match the "style".  Tracking the current ring size
>>>> and avoiding reallocating a potentially large chunk of memory seems like
>>>> a better idea.
>>>>
>>>>> Also the ring_dma_zone_reserve() could get replaced by
>>>>> rte_eth_dma_zone_reserve() (see also
>>>>
>>>>
>>>>
>>>> Yes, it probably should get changed to that along with tracking the size.
>>>
>>>
>>> Why don't we always allocate VMXNET3_RX_RING_MAX_SIZE entries? That
>>> way we don't need to reallocate on a later queue setup change?
>>
>>
>> That's using more memory than it needs to use.  It might break someone's
>> application
>> that already runs in some tightly constrained machine.  Failing to shrink
>> the memzone
>> isn't likely to break anything since they have (apparently) already dealt
>> with having
>> less memory available before switching to a smaller queue size.
>>
>> Still, it can be a matter for another day.
>>
>
> Other drivers (ixgbe, e1000, ...) always allocate based on the max
> ring size too. Since the VMXNET3_RX_RING_MAX_SIZE is 4096 I don't
> think it is a huge waste of memory.

I changed my mind.  Our typical application only uses a queue length of
512 for the TX side which would mean we don't use more than half of the
allocated elements.  It's easy enough to just allocate what we need and
release in the queue releases as you suggested earlier.

diff mbox

Patch

diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index 6649c3f..104e040 100644
--- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
+++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
@@ -893,8 +893,8 @@  vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 
 /*
  * Create memzone for device rings. malloc can't be used as the physical address is
- * needed. If the memzone is already created, then this function returns a ptr
- * to the old one.
+ * needed. If the memzone already exists, we free it since it may have been created
+ * with a different size.
  */
 static const struct rte_memzone *
 ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
@@ -909,7 +909,7 @@  ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
 
 	mz = rte_memzone_lookup(z_name);
 	if (mz)
-		return mz;
+		rte_memzone_free(mz);
 
 	return rte_memzone_reserve_aligned(z_name, ring_size,
 					   socket_id, 0, VMXNET3_RING_BA_ALIGN);