RE: [PATCH v3] pcap: support MTU set

[Ido Goshen <Ido@cgstowernetworks.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
> Sent: Monday, 30 May 2022 21:06
> To: Ido Goshen <Ido@cgstowernetworks.com>; ferruh.yigit@xilinx.com;
> stephen@networkplumber.org
> Cc: dev@dpdk.org; Tianli Lai <laitianli@tom.com>
> Subject: Re: [PATCH v3] pcap: support MTU set
> 
> On 5/30/2022 11:36 AM, Ido Goshen wrote:
> > Support rte_eth_dev_set_mtu by pcap vdevs Enforce mtu on rx/tx
> >
> 
> Still not sure about enforcing MTU on pcap, but please find comments on
> mechanical issues

[idog] Trying to detail more about use cases:
1. CI tests which are HW independent and works with --vdevs=net_pcap
For testing that mtu feature(s) work correctly in our app a pcap that contains
mix sized packets should be used. Using pcap that has only small size packets
will miss the all point.
2. Customer support - it's much simpler to debug on a workstation and not 
real HW setup. We often get the customer's configuration and a pcap in order
to reproduce an issue. It will be a pain and error-prone to manipulate the pcap
before using it.

I will address the mechanical issues and post v4 patch

> > Bugzilla ID: 961
> > Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> >
> > ---
> > v3:
> > Preserve pcap behavior to support max size packets by default
> > alternative to v2 in order to limit the code change to pcap only and
> > avoid abi change.
> > Enforce mtu only in case rte_eth_dev_set_mtu was explicitly called.
> >
> > v2:
> > Preserve pcap behavior to support max size packets by default.
> > ---
> >   drivers/net/pcap/pcap_ethdev.c | 44
> +++++++++++++++++++++++++++++++---
> >   1 file changed, 41 insertions(+), 3 deletions(-)
> >
> 
> Is documentation needs to be updated as well?
 
[idog] I don't think so 
It's using the standard rte_eth_dev_set_mtu() which is already documented in 
features.rst https://doc.dpdk.org/guides/nics/features.html#mtu-update
I don't see other PMDs mention explicitly they support it (it's the normal behaviour)

> And what do you think to update release notes for this update?
[idog] ok

> > +388,8 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs,
> uint16_t nb_pkts)
> > +		if (unlikely(len > dev->data->mtu) &&
> > +				internals->is_mtu_set) {
> 
> It is possible to save only some part of the packet to the pcap file, please
> check snaplen patch [1], how MTU config should work with this feature?
> 
> [1]
> https://patchwork.dpdk.org/project/dpdk/patch/20220313112638.3945-1-
> laitianli@tom.com/
> 

[idog] interesting to know this is being work on.
(To your method, why is it needed?  there are tools like editpcap that can be
applied on the dpdk output file and snap the packets ;-)
I think integration of the 2 features is trivial, mtu controls if the packet is 
written to file or not, and snaplan controls what part of it needs to be written. 
i.e. mtu is checked before snaplen. 
Using snaplen > mtu will become meaningless (maybe block/warn on such configuration)

Alternative is to apply mtu only on pcap iface (assuming snaplen is applied only
 on tx_pcap file)

If supporting mtu only for pcap live ifaces and not for pcap files then
the all thing can be implemented differently by setting the OS netdevice 
mtu (e.g. in linux SIOCSIFMTU) instead of enforcing it by the pcap pmd
but this will require osdep use and I admit I have no idea how to do it for windows
Would this be a better approach?