Re: [dpdk-dev] [PATCH 1/3] mbuf:add two TX offload flags and change three fields

["Liu, Jijiang" <jijiang.liu@intel.com>]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Sunday, November 30, 2014 10:51 PM
> To: Olivier MATZ; Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 1/3] mbuf:add two TX offload flags and change
> three fields
> 
> Hi Olver,
> 
> > -----Original Message-----
> > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > Sent: Friday, November 28, 2014 10:46 AM
> > To: Ananyev, Konstantin; Liu, Jijiang
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 1/3] mbuf:add two TX offload flags and
> > change three fields
> >
> > Hi Konstantin,
> >
> > On 11/27/2014 06:01 PM, Ananyev, Konstantin wrote:
> > >> Yes, I think it could be done that way too.
> > >> Though I still prefer to keep l4tun_len - it makes things a bit cleaner (at least
> to me).
> > >> After all  we do have space for it in mbuf's tx_offload.
> > >
> > > As one more thing in favour of separate l4tun_len field:
> > > l2_len is 7 bit long, so in theory it might be not enough, as for FVL:
> > > 12:18 L4TUNLEN L4 Tunneling Length (Teredo / GRE header / VXLAN header)
> defined in Words.
> >
> > The l2_len field is 7 bits long because it was mapped to ixgbe hardware.
> > If it's not enough (although I'm not sure it's possible to have a
> > header larger than 128 bytes in this case), it's probably because we
> > should not have been doing that.
> > Maybe these generic fields should be generic :) If it's not enough,
> > what about changing l2_len to 8 bits?
> >
> > Often in the recent discussions, I see as an argument "fortville needs
> > this so we need to add it in the mbuf". I agree that currently
> > fortville is the only hardware supported for the new features, so it
> > can make sense to act like this, but we have to accept to come back to
> > this API in the future if it makes less sense with other drivers.
> >
> > Also, application developers can be annoyed to see that the mbuf flags
> > and meta data are just duplicating information that is already present
> > in the packet.
> >
> > About the l4tun_len, it's another field the application has to fill,
> > but it's maybe cleaner. I just wanted to clarify why I'm discussing
> > these points.
> 
> After another thought, I think that the way you proposed is a better one.
> I gives us more flexibility:
> let's say for now we'll keep both l2_len and outer_l2_len as 7 bit fields, and upper
> layer would have to:
> mb->l2_len =  eth_hdr_in + vxlan_hdr;

A question, how to fill the mb->l2_len  for IP in IP, IP in GRE tunneling packet that is no inner L2 header?
What  is the rule?
Or you think it should be mb->l2_len =  0 + l4_tun_len;  ??

> for VXLAN packets.
> Then if in the future, we'll realise that 7 bit is not enough we can either:
> - increase size of l2_len and outer_l2_len
> - introduce new field (l4tun_len as we planned now) In both cases backward
> compatibility wouldn't be affected.
> From other side - if we'' introduce a new field now (l4tun_len), it would be very
> hard to get rid of it in the future.
> So, I think we'd better follow your approach here.
> 
> Thanks
> Konstantin
> 
> 
> >
> > Regards,
> > Olivier