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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

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;
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