[4/5] net/iavf: fix protocol size for virtchnl copy
Checks
Commit Message
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(-)
Comments
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_*'...
btw, while checking for the 'struct rte_flow_item_eth', pahole shows it is using
20 bytes, and I suspect this is not the intention with the reserved field:
struct rte_flow_item_eth {
struct rte_ether_addr dst; /* 0 6 */
struct rte_ether_addr src; /* 6 6 */
uint16_t type; /* 12 2 */
/* Bitfield combined with previous fields */
uint32_t has_vlan:1; /* 12:15 4 */
/* XXX 31 bits hole, try to pack */
uint32_t reserved:31; /* 16: 1 4 */
/* size: 20, cachelines: 1, members: 5 */
/* bit holes: 1, sum bit holes: 31 bits */
/* bit_padding: 1 bits */
/* last cacheline: 20 bytes */
};
'has_vlan' seems combined with previous field to make together 32 bits. So the
'reserved' field is occupying a new 32 bits all by itself.
What about changing the struct as following, while we can change the ABI:
struct rte_flow_item_eth {
struct rte_ether_addr dst; /* 0 6 */
struct rte_ether_addr src; /* 6 6 */
uint16_t type; /* 12 2 */
uint16_t has_vlan:1; /* 14:15 2 */
uint16_t reserved:15; /* 14: 0 2 */
/* size: 16, cachelines: 1, members: 5 */
/* last cacheline: 16 bytes */
};
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, November 17, 2020 00:23
> To: Xiaoyu Min <jackmin@mellanox.com>; Jingjing Wu <jingjing.wu@intel.com>;
> Beilei Xing <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Jack Min <jackmin@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Ori Kam <orika@nvidia.com>; Dekel Peled
> <dekelp@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
>
> 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.
>
>
> btw, while checking for the 'struct rte_flow_item_eth', pahole shows it is using
> 20 bytes, and I suspect this is not the intention with the reserved field:
>
> struct rte_flow_item_eth {
> struct rte_ether_addr dst; /* 0 6 */
> struct rte_ether_addr src; /* 6 6 */
> uint16_t type; /* 12 2 */
>
> /* Bitfield combined with previous fields */
>
> uint32_t has_vlan:1; /* 12:15 4 */
>
> /* XXX 31 bits hole, try to pack */
>
> uint32_t reserved:31; /* 16: 1 4 */
>
> /* size: 20, cachelines: 1, members: 5 */
> /* bit holes: 1, sum bit holes: 31 bits */
> /* bit_padding: 1 bits */
> /* last cacheline: 20 bytes */
> };
>
> 'has_vlan' seems combined with previous field to make together 32 bits. So the
> 'reserved' field is occupying a new 32 bits all by itself.
>
> What about changing the struct as following, while we can change the ABI:
> struct rte_flow_item_eth {
> struct rte_ether_addr dst; /* 0 6 */
> struct rte_ether_addr src; /* 6 6 */
> uint16_t type; /* 12 2 */
> uint16_t has_vlan:1; /* 14:15 2 */
> uint16_t reserved:15; /* 14: 0 2 */
>
> /* size: 16, cachelines: 1, members: 5 */
> /* last cacheline: 16 bytes */
> };
>
Well we probably need to discuss this in next release.
It's too late to change this API at this moment.
-Jack
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.
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.
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>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
23/11/2020 11:49, Ferruh Yigit:
> 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>
>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Applied
> -----Original Message-----
> From: Xiaoyu Min <jackmin@mellanox.com>
> Sent: Monday, November 16, 2020 3:55 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Xiaoyu Min <jackmin@nvidia.com>
> Subject: [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
>
> 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>
Acked-by: Beilei Xing <beilei.xing@intel.com>
@@ -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));
}
filter->add_fltr.rule_cfg.proto_hdrs.count = ++layer;