Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/22/2020 2:15 PM, Thomas Monjalon wrote:
> 22/11/2020 14:28, Jack Min:
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
>>>> From: Xiaoyu Min <jackmin@nvidia.com>
>>>>
>>>> The rte_flow_item_vlan items are refined.
>>>> The structs do not exactly represent the packet bits captured on the
>>>> wire anymore so should only copy real header instead of the whole struct.
>>>>
>>>> Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
>>>>
>>>> Fixes: 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
>>> items")
>>>>
>>>> Signed-off-by: Xiaoyu Min <jackmin@nvidia.com>
>>>> ---
>>>>    drivers/net/iavf/iavf_fdir.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/iavf/iavf_fdir.c b/drivers/net/iavf/iavf_fdir.c
>>>> index d683a468c1..7054bde0b9 100644
>>>> --- a/drivers/net/iavf/iavf_fdir.c
>>>> +++ b/drivers/net/iavf/iavf_fdir.c
>>>> @@ -541,7 +541,7 @@ iavf_fdir_parse_pattern(__rte_unused struct
>>> iavf_adapter *ad,
>>>>    				VIRTCHNL_ADD_PROTO_HDR_FIELD_BIT(hdr,
>>> ETH, ETHERTYPE);
>>>>
>>>>    				rte_memcpy(hdr->buffer,
>>>> -					eth_spec, sizeof(*eth_spec));
>>>> +					eth_spec, sizeof(struct rte_ether_hdr));
>>>
>>> This requires 'struct rte_flow_item_eth' should have 'struct rte_ether_hdr' as
>>> first element, and I suspect this usage exists in a few more locations, but I
>>> wonder if this assumption is real and documented in somewhere?
>>> I am not just talking about 'struct rte_flow_item_eth', but for all
>>> 'rte_flow_item_*'...
>>>
>> I think this is not documented and this assumption is not real.
>> I've created one ticket on Bugzilla (https://bugs.dpdk.org/show_bug.cgi?id=581) to track.
> 
> So it probably means this patch is not enough for iavf.
> It would require iavf maintainers (Cc'ed) to follow up.
> I will apply the rest of the series.
> 


I think the iavf patch is OK.

My comment was general, on how new fields added to the "struct 
rte_flow_item_eth", which is causing 4 bytes of waste unintentionally.

And if we don't fix it in this release, we won't able to fix it until next release.

I think first thing is to get the iavf fix.

Later we can discuss to get the struct change in this release or not.