Re: [dpdk-dev] [PATCH v3 07/10] vmxnet3: support jumbo frames

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 11 Mar 2015 01:03:37 +0000
Yong Wang <yongwang@vmware.com> wrote:

> >Looking in more detail, the code as submitted looks fine:
> >
> >* The # of rx descriptors needed for each packet is responsibility of
> >  the application. Before and after this patch the # of Rx descriptors
> >  for each ring (head/body) is determined by vmxnet3_rx_queue_setup.
> >  And the driver always allocates the same number of descriptors
> >  for both Rx rings.  
> 
> Where is vmxnet3_rx_queue_setup? It’s not in the current code base nor in
> your patch and I failed to see how rx descriptors are populated.



> Could you explain what do you mean by responsibility of the application?
> If mtu is set to 9000, the driver needs to make sure rx buf per packet is
> set to the correct size with proper type set, right?

vmxnet3_dev_rx_queue_setup is unchanged from original Intel code.

But you are right there is a bug there since it will error out
if max_rx_pkt_len > bufsize.  That error check should have been removed.
That was an oversight from the merge of our code with the DPDK/Intel
code. Just removing the check and surrounding code is all that is necessary.

After that driver will receive scattered mbufs.

None of the DPDK drivers do a good job of checking all the possible
misconfiguration of rxmode, jumbo and scattered flags. There is room
for improvement in all drivers around that.


> >  One could argue that was a bug in the original driver, since it
> >needlessly
> >  allocated mbufs for the second ring and never used them. I suppose
> >  as a optimization in future, the second ring could be sized as 0
> >  and no buffers allocated unless doing rx-scatter (this is what the
> >  Linux driver does. But that change is independent of this.  
> 
> We can just keep it as is as we will need it if someone decides to support
> LRO for vmxnet3.

But LRO must be optional since LRO is broken for the case of forwarding.
Linux driver handles the case of only allocating second ring if necessary.
Although the Linux vmxnet3 driver was broken about LRO setting for
several years, it is fixed now.


> >
> >* Buffers are correctly reallocated as needed when consumed.
> >
> >* The code works for bulk transfer and performance tests in jumbo mode.  
> 
> You mean the standalone patch you posted without the
> vmxnet3_rx_queue_setup you mentioned?

The code in recv that calls vmxnet3_post_rx_bufs works for both rings
already. It was just until the scattered logic was engaged the second
ring was never used.