| Message ID | 20230512152607.992209-11-larysa.zaremba@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | new kfunc XDP hints and ice implementation | expand |
On 05/12, Larysa Zaremba wrote: > Implement .xmo_rx_vlan_tag callback to allow XDP code to read > packet's VLAN tag. > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 44 +++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > index 1caa73644e7b..39547feb6106 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > @@ -627,7 +627,51 @@ static int ice_xdp_rx_hash(const struct xdp_md *ctx, u32 *hash, > return 0; > } > > +/** > + * ice_xdp_rx_ctag - VLAN tag XDP hint handler > + * @ctx: XDP buff pointer > + * @vlan_tag: destination address > + * > + * Copy VLAN tag (if was stripped) to the destination address. > + */ > +static int ice_xdp_rx_ctag(const struct xdp_md *ctx, u16 *vlan_tag) > +{ > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > + netdev_features_t features; > + [..] > + features = xdp_ext->rx_ring->netdev->features; > + > + if (!(features & NETIF_F_HW_VLAN_CTAG_RX)) > + return -EINVAL; Passing-by comment: why do we need to check features? ice_get_vlan_tag_from_rx_desc seems to be checking a bunch of fields in the descriptors, so that should be enough? > + > + *vlan_tag = ice_get_vlan_tag_from_rx_desc(xdp_ext->eop_desc); Should we also do the following: if (!*vlan_tag) return -ENODATA; ? > + return 0; > +} > + > +/** > + * ice_xdp_rx_stag - VLAN s-tag XDP hint handler > + * @ctx: XDP buff pointer > + * @vlan_tag: destination address > + * > + * Copy VLAN s-tag (if was stripped) to the destination address. > + */ > +static int ice_xdp_rx_stag(const struct xdp_md *ctx, u16 *vlan_tag) > +{ > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > + netdev_features_t features; > + > + features = xdp_ext->rx_ring->netdev->features; > + > + if (!(features & NETIF_F_HW_VLAN_STAG_RX)) > + return -EINVAL; > + > + *vlan_tag = ice_get_vlan_tag_from_rx_desc(xdp_ext->eop_desc); > + return 0; > +} > + > const struct xdp_metadata_ops ice_xdp_md_ops = { > .xmo_rx_timestamp = ice_xdp_rx_hw_ts, > .xmo_rx_hash = ice_xdp_rx_hash, > + .xmo_rx_ctag = ice_xdp_rx_ctag, > + .xmo_rx_stag = ice_xdp_rx_stag, > }; > -- > 2.35.3 >
On Fri, May 12, 2023 at 11:31:21AM -0700, Stanislav Fomichev wrote: > On 05/12, Larysa Zaremba wrote: > > Implement .xmo_rx_vlan_tag callback to allow XDP code to read > > packet's VLAN tag. > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 44 +++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > index 1caa73644e7b..39547feb6106 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > @@ -627,7 +627,51 @@ static int ice_xdp_rx_hash(const struct xdp_md *ctx, u32 *hash, > > return 0; > > } > > > > +/** > > + * ice_xdp_rx_ctag - VLAN tag XDP hint handler > > + * @ctx: XDP buff pointer > > + * @vlan_tag: destination address > > + * > > + * Copy VLAN tag (if was stripped) to the destination address. > > + */ > > +static int ice_xdp_rx_ctag(const struct xdp_md *ctx, u16 *vlan_tag) > > +{ > > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > > + netdev_features_t features; > > + > > [..] > > > + features = xdp_ext->rx_ring->netdev->features; > > + > > + if (!(features & NETIF_F_HW_VLAN_CTAG_RX)) > > + return -EINVAL; > > Passing-by comment: why do we need to check features? > ice_get_vlan_tag_from_rx_desc seems to be checking a bunch of > fields in the descriptors, so that should be enough? Unfortunately, it is not enough, because it only checks, if there is a valid value in the descriptor, without distinguishing c-tag from s-tag. In this hardware, c-tag and s-tag are mutually exclusive, so they can occupy same descriptor fields. Checking netdev features is just the easiest way to tell them apart. I guess, storing this information in in the ring structure would be more efficient than checking netdev features. I know Piotr Raczynski indends to review this series, so maybe he would provide some additional feedback/suggestions. > > > + > > + *vlan_tag = ice_get_vlan_tag_from_rx_desc(xdp_ext->eop_desc); > > Should we also do the following: > > if (!*vlan_tag) > return -ENODATA; > > ? Oh, returning VLAN tag with zero value really made sense to me at the beginning, but after playing with different kinds of packets, I think returning error makes more sense. Will change. > > > + return 0; > > +} > > + > > +/** > > + * ice_xdp_rx_stag - VLAN s-tag XDP hint handler > > + * @ctx: XDP buff pointer > > + * @vlan_tag: destination address > > + * > > + * Copy VLAN s-tag (if was stripped) to the destination address. > > + */ > > +static int ice_xdp_rx_stag(const struct xdp_md *ctx, u16 *vlan_tag) > > +{ > > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > > + netdev_features_t features; > > + > > + features = xdp_ext->rx_ring->netdev->features; > > + > > + if (!(features & NETIF_F_HW_VLAN_STAG_RX)) > > + return -EINVAL; > > + > > + *vlan_tag = ice_get_vlan_tag_from_rx_desc(xdp_ext->eop_desc); > > + return 0; > > +} > > + > > const struct xdp_metadata_ops ice_xdp_md_ops = { > > .xmo_rx_timestamp = ice_xdp_rx_hw_ts, > > .xmo_rx_hash = ice_xdp_rx_hash, > > + .xmo_rx_ctag = ice_xdp_rx_ctag, > > + .xmo_rx_stag = ice_xdp_rx_stag, > > }; > > -- > > 2.35.3 > >
On 15/05/2023 15.41, Larysa Zaremba wrote: >>> + *vlan_tag = ice_get_vlan_tag_from_rx_desc(xdp_ext->eop_desc); >> Should we also do the following: >> >> if (!*vlan_tag) >> return -ENODATA; >> >> ? > Oh, returning VLAN tag with zero value really made sense to me at the beginning, > but after playing with different kinds of packets, I think returning error makes > more sense. Will change. > IIRC then VLAN tag zero is also a valid id, right? --Jesper
On Mon, May 15, 2023 at 05:07:19PM +0200, Jesper Dangaard Brouer wrote: > > > On 15/05/2023 15.41, Larysa Zaremba wrote: > > > > + *vlan_tag = ice_get_vlan_tag_from_rx_desc(xdp_ext->eop_desc); > > > Should we also do the following: > > > > > > if (!*vlan_tag) > > > return -ENODATA; > > > > > > ? > > Oh, returning VLAN tag with zero value really made sense to me at the beginning, > > but after playing with different kinds of packets, I think returning error makes > > more sense. Will change. > > > > IIRC then VLAN tag zero is also a valid id, right? AFAIK, 0x000 is reseved and basically means "no vlan tag". When ice hardware returns such value in descriptor, it says "no vlan tag was stripped" and this doesn't necessarily mean there is no VLAN tag in the packet. For example, let us consider a packet: Ether/802.1ad(s-tag)/802.1q(c-tag)/... Hardware does not strip c-tag in such case and sends 0x000 in the descriptor, but packet clearly does contain a c-tag, so at least in ice, it is reasonable to not consider '0' a reliable value. I guess, for s-tag value of 0x000 should be more reliable, so maybe 'if (!*vlan_tag)' usage can be limited to c-tag function. > > --Jesper >
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c index 1caa73644e7b..39547feb6106 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c @@ -627,7 +627,51 @@ static int ice_xdp_rx_hash(const struct xdp_md *ctx, u32 *hash, return 0; } +/** + * ice_xdp_rx_ctag - VLAN tag XDP hint handler + * @ctx: XDP buff pointer + * @vlan_tag: destination address + * + * Copy VLAN tag (if was stripped) to the destination address. + */ +static int ice_xdp_rx_ctag(const struct xdp_md *ctx, u16 *vlan_tag) +{ + const struct ice_xdp_buff *xdp_ext = (void *)ctx; + netdev_features_t features; + + features = xdp_ext->rx_ring->netdev->features; + + if (!(features & NETIF_F_HW_VLAN_CTAG_RX)) + return -EINVAL; + + *vlan_tag = ice_get_vlan_tag_from_rx_desc(xdp_ext->eop_desc); + return 0; +} + +/** + * ice_xdp_rx_stag - VLAN s-tag XDP hint handler + * @ctx: XDP buff pointer + * @vlan_tag: destination address + * + * Copy VLAN s-tag (if was stripped) to the destination address. + */ +static int ice_xdp_rx_stag(const struct xdp_md *ctx, u16 *vlan_tag) +{ + const struct ice_xdp_buff *xdp_ext = (void *)ctx; + netdev_features_t features; + + features = xdp_ext->rx_ring->netdev->features; + + if (!(features & NETIF_F_HW_VLAN_STAG_RX)) + return -EINVAL; + + *vlan_tag = ice_get_vlan_tag_from_rx_desc(xdp_ext->eop_desc); + return 0; +} + const struct xdp_metadata_ops ice_xdp_md_ops = { .xmo_rx_timestamp = ice_xdp_rx_hw_ts, .xmo_rx_hash = ice_xdp_rx_hash, + .xmo_rx_ctag = ice_xdp_rx_ctag, + .xmo_rx_stag = ice_xdp_rx_stag, };
Implement .xmo_rx_vlan_tag callback to allow XDP code to read packet's VLAN tag. Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> --- drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+)