From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
To: Artemii Morozov <artemii.morozov@arknetworks.am>, dev@dpdk.org
Cc: Ivan Malov <ivan.malov@arknetworks.am>,
Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>,
Andy Moreton <amoreton@xilinx.com>
Subject: Re: [PATCH v4 2/3] common/sfc_efx/base: add support to enable VLAN stripping
Date: Fri, 2 Jun 2023 11:32:17 +0300 [thread overview]
Message-ID: <c4d72eb1-86a3-b797-9f6b-c2ae485c958b@oktetlabs.ru> (raw)
In-Reply-To: <20230601153022.99634-3-artemii.morozov@arknetworks.am>
On 6/1/23 18:30, Artemii Morozov wrote:
> To enable VLAN stripping, two conditions must be met:
> the corresponding flag must be set and the appropriate
> RX prefix should be requested.
RX -> Rx
> VLAN stripping is supported for ef100 datapath only.
When you read below notes carefully, you'll understand that the
patch is very raw yet. There are too many questions below.
The major decision to be made is if libefx API should enforce
device level offload or should just provide transparent API and
driver must be responsible for consistency.
If driver must be responsible for consistency, don't try to
enforce VLAN stripping enable via filter table. RxQ flags
should control stripped VLAN delivery. Filter flag and default
RxQ set flag (API should be extended) should control VLAN
stripping enabling.
If libefx must guarantee consistency, we need a new way to
enable device level offload early (efx_rx_init()-like, may
be a new API). If so, it should be impossible to control
VLAN stripping per Rx filter. It still could be possible
to control stripped VLAN delivery per RxQ.
Anyway current solution is really inconsistent.
>
> Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
> Reviewed-by: Ivan Malov <ivan.malov@arknetworks.am>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
> drivers/common/sfc_efx/base/ef10_filter.c | 21 +++++++++++++++++++++
> drivers/common/sfc_efx/base/ef10_impl.h | 1 +
> drivers/common/sfc_efx/base/efx.h | 9 +++++++++
> drivers/common/sfc_efx/base/efx_filter.c | 3 ++-
> drivers/common/sfc_efx/base/efx_impl.h | 1 +
> drivers/common/sfc_efx/base/efx_rx.c | 17 +++++++++++++++++
> drivers/common/sfc_efx/base/rhead_rx.c | 3 +++
> 7 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/common/sfc_efx/base/ef10_filter.c b/drivers/common/sfc_efx/base/ef10_filter.c
> index 6d19797d16..6371d8d489 100644
> --- a/drivers/common/sfc_efx/base/ef10_filter.c
> +++ b/drivers/common/sfc_efx/base/ef10_filter.c
> @@ -338,6 +338,11 @@ efx_mcdi_filter_op_add(
> MC_CMD_FILTER_OP_V3_IN_MATCH_ACTION_FLAG);
> }
>
> + if (spec->efs_flags & EFX_FILTER_FLAG_VLAN_STRIP) {
> + MCDI_IN_SET_DWORD_FIELD(req, FILTER_OP_V3_IN_MATCH_ACTION_FLAGS,
> + FILTER_OP_V3_IN_MATCH_STRIP_VLAN, 1);
> + }
> +
> efx_mcdi_execute(enp, &req);
>
> if (req.emr_rc != 0) {
> @@ -852,6 +857,16 @@ ef10_filter_add_internal(
>
> hash = ef10_filter_hash(spec);
>
> + /*
> + * VLAN stripping is offload at the device level, and it should be
> + * applied to all filters if it has been applied at least once before.
> + * Knowledge of device level vlan strip offload being enabled comes from
vlan -> VLAN
> + * the default RxQ flags, albeit the flag may be set on any queue.
> + * But only default RxQ affects this 'eft_strip_vlan' decision.
> + */
> + if (eftp->eft_strip_vlan)
> + spec->efs_flags |= EFX_FILTER_FLAG_VLAN_STRIP;
> +
> /*
> * FIXME: Add support for inserting filters of different priorities
> * and removing lower priority multicast filters (bug 42378)
> @@ -2010,6 +2025,9 @@ ef10_filter_reconfigure(
> else
> filter_flags = 0;
>
> + if (table->eft_strip_vlan)
> + filter_flags |= EFX_FILTER_FLAG_VLAN_STRIP;
> +
> /* Mark old filters which may need to be removed */
> ef10_filter_mark_old_filters(enp);
>
> @@ -2142,6 +2160,8 @@ ef10_filter_default_rxq_set(
> EFSYS_ASSERT(using_rss == B_FALSE);
> table->eft_using_rss = B_FALSE;
> #endif
> + if (erp->er_flags & EFX_RXQ_FLAG_VLAN_STRIP)
> + table->eft_strip_vlan = B_TRUE;
How to enable VLAN stripping in isolated mode where
API to set default RxQ is not called?
> table->eft_default_rxq = erp;
> }
>
> @@ -2153,6 +2173,7 @@ ef10_filter_default_rxq_clear(
>
> table->eft_default_rxq = NULL;
> table->eft_using_rss = B_FALSE;
> + table->eft_strip_vlan = B_FALSE;
> }
>
>
> diff --git a/drivers/common/sfc_efx/base/ef10_impl.h b/drivers/common/sfc_efx/base/ef10_impl.h
> index 877aedad45..017e561f19 100644
> --- a/drivers/common/sfc_efx/base/ef10_impl.h
> +++ b/drivers/common/sfc_efx/base/ef10_impl.h
> @@ -1287,6 +1287,7 @@ typedef struct ef10_filter_table_s {
> ef10_filter_entry_t eft_entry[EFX_EF10_FILTER_TBL_ROWS];
> efx_rxq_t *eft_default_rxq;
> boolean_t eft_using_rss;
> + boolean_t eft_strip_vlan;
> uint32_t eft_unicst_filter_indexes[
> EFX_EF10_FILTER_UNICAST_FILTERS_MAX];
> uint32_t eft_unicst_filter_count;
> diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
> index ac89b418e0..508d0a802d 100644
> --- a/drivers/common/sfc_efx/base/efx.h
> +++ b/drivers/common/sfc_efx/base/efx.h
> @@ -3101,6 +3101,10 @@ typedef enum efx_rxq_type_e {
> * Request user flag field in the Rx prefix of a queue.
> */
> #define EFX_RXQ_FLAG_USER_FLAG 0x20
> +/*
> + * Request vlan tci field in the Rx prefix of a queue.
vlan -> VLAN, tci -> TCI
The name and comment are a bit misleading. Name sounds like it
controls VLAN stripping on the RxQ.
Consider EFX_RXQ_FLAG_STRIPPED_VLAN to highlight that it
just requests stripped VLAN TCI. Moreover, comment should
say that it does *not* control VLAN stripping. It just
controls delivery of the stripped VLAN TCI if VLAN stripping
is enabled and done for a packet.
However, as far as I can see finally the flag is used to
enable VLAN stripping as well and it is terribly bad since
it adds false expectations on per-RxQ control for the feature.
It the flag should really control Rx VLAN stripping,
comment should say so and explain limitations.
> + */
> +#define EFX_RXQ_FLAG_VLAN_STRIP 0x40
>
> LIBEFX_API
> extern __checkReturn efx_rc_t
> @@ -3455,6 +3459,11 @@ efx_tx_qdestroy(
> #define EFX_FILTER_FLAG_ACTION_FLAG 0x20
> /* Set match mark on the received packet */
> #define EFX_FILTER_FLAG_ACTION_MARK 0x40
> +/*
> + * Request that the first tag in the outer L2 header be stripped
> + * and TCI be indicated in Rx prefix.
> + */
> +#define EFX_FILTER_FLAG_VLAN_STRIP 0x80
The flag allows driver to request Rx VLAN stripping in
some filters only. Is it really intended?
>
> typedef uint8_t efx_filter_flags_t;
>
> diff --git a/drivers/common/sfc_efx/base/efx_filter.c b/drivers/common/sfc_efx/base/efx_filter.c
> index 83c37ff859..778ed0c370 100644
> --- a/drivers/common/sfc_efx/base/efx_filter.c
> +++ b/drivers/common/sfc_efx/base/efx_filter.c
> @@ -322,7 +322,8 @@ efx_filter_spec_init_rx(
> EFSYS_ASSERT3P(spec, !=, NULL);
> EFSYS_ASSERT3P(erp, !=, NULL);
> EFSYS_ASSERT((flags & ~(EFX_FILTER_FLAG_RX_RSS |
> - EFX_FILTER_FLAG_RX_SCATTER)) == 0);
> + EFX_FILTER_FLAG_RX_SCATTER |
> + EFX_FILTER_FLAG_VLAN_STRIP)) == 0);
>
> memset(spec, 0, sizeof (*spec));
> spec->efs_priority = priority;
> diff --git a/drivers/common/sfc_efx/base/efx_impl.h b/drivers/common/sfc_efx/base/efx_impl.h
> index 45e99d01c5..c45669e077 100644
> --- a/drivers/common/sfc_efx/base/efx_impl.h
> +++ b/drivers/common/sfc_efx/base/efx_impl.h
> @@ -1045,6 +1045,7 @@ struct efx_rxq_s {
> efsys_mem_t *er_esmp;
> efx_evq_rxq_state_t *er_ev_qstate;
> efx_rx_prefix_layout_t er_prefix_layout;
> + uint16_t er_flags;
Why is it 16-bit width. It shares typedefs with RxQ create
flags which are unsigned int?
> };
>
> #define EFX_RXQ_MAGIC 0x15022005
> diff --git a/drivers/common/sfc_efx/base/efx_rx.c b/drivers/common/sfc_efx/base/efx_rx.c
> index 68f42f5cac..5726085953 100644
> --- a/drivers/common/sfc_efx/base/efx_rx.c
> +++ b/drivers/common/sfc_efx/base/efx_rx.c
> @@ -925,6 +925,7 @@ efx_rx_qcreate_internal(
> erp->er_index = index;
> erp->er_mask = ndescs - 1;
> erp->er_esmp = esmp;
> + erp->er_flags = 0;
>
> if ((rc = erxop->erxo_qcreate(enp, index, label, type, type_data, esmp,
> ndescs, id, flags, eep, erp)) != 0)
> @@ -941,11 +942,27 @@ efx_rx_qcreate_internal(
As far as I can see it is a bug here since rc is 0, but the
function should fail. Please, submit separate patch with
appropriate Fixes tag.
> goto fail5;
> }
>
> + if (flags & EFX_RXQ_FLAG_VLAN_STRIP) {
> + const efx_rx_prefix_layout_t *erplp = &erp->er_prefix_layout;
> + const efx_rx_prefix_field_info_t *vlan_tci_field;
> +
> + vlan_tci_field =
> + &erplp->erpl_fields[EFX_RX_PREFIX_FIELD_VLAN_STRIP_TCI];
> + if (vlan_tci_field->erpfi_width_bits == 0) {
> + rc = ENOTSUP;
> + goto fail6;
> + }
IMHO support for Rx VLAN stripping capability should be
checked here as well. It is different aspects of the HW
support:
1. Support for Rx VLAN stripping
2. Delivery of the stripped VLAN tag
> +
> + erp->er_flags |= EFX_RXQ_FLAG_VLAN_STRIP;
> + }
> +
> enp->en_rx_qcount++;
> *erpp = erp;
>
> return (0);
>
> +fail6:
> + EFSYS_PROBE(fail6);
> fail5:
> EFSYS_PROBE(fail5);
>
next prev parent reply other threads:[~2023-06-02 8:32 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-31 13:41 [PATCH 0/3] net/sfc: support VLAN stripping offload Artemii Morozov
2023-05-31 13:41 ` [PATCH 1/3] common/sfc_efx/base: report VLAN stripping capability Artemii Morozov
2023-06-01 8:12 ` [PATCH v2 0/3] net/sfc: support VLAN stripping offload Artemii Morozov
2023-06-01 8:12 ` [PATCH v2 1/3] common/sfc_efx/base: report VLAN stripping capability Artemii Morozov
2023-06-01 14:35 ` [PATCH v3 0/3] net/sfc: support VLAN stripping offload Artemii Morozov
2023-06-01 14:35 ` [PATCH v3 1/3] common/sfc_efx/base: report VLAN stripping capability Artemii Morozov
2023-06-01 14:35 ` [PATCH v3 2/3] common/sfc_efx/base: add support to enable VLAN stripping Artemii Morozov
2023-06-01 14:35 ` [PATCH v3 3/3] net/sfc: support VLAN stripping offload Artemii Morozov
2023-06-01 8:12 ` [PATCH v2 2/3] common/sfc_efx/base: add support to enable VLAN stripping Artemii Morozov
2023-06-01 8:12 ` [PATCH v2 3/3] net/sfc: support VLAN stripping offload Artemii Morozov
2023-05-31 13:41 ` [PATCH 2/3] common/sfc_efx/base: add support to enable VLAN stripping Artemii Morozov
2023-05-31 13:41 ` [PATCH 3/3] net/sfc: support VLAN stripping offload Artemii Morozov
2023-06-01 15:30 ` [PATCH v4 0/3] " Artemii Morozov
2023-06-01 15:30 ` [PATCH v4 1/3] common/sfc_efx/base: report VLAN stripping capability Artemii Morozov
2023-06-02 7:22 ` Andrew Rybchenko
2023-06-01 15:30 ` [PATCH v4 2/3] common/sfc_efx/base: add support to enable VLAN stripping Artemii Morozov
2023-06-02 8:32 ` Andrew Rybchenko [this message]
2023-06-08 11:16 ` Artemii Morozov
2023-06-08 12:37 ` Andrew Rybchenko
2023-06-01 15:30 ` [PATCH v4 3/3] net/sfc: support VLAN stripping offload Artemii Morozov
2023-06-02 8:46 ` Andrew Rybchenko
2023-06-13 15:12 ` [PATCH v5 0/3] " Artemii Morozov
2023-06-13 15:12 ` [PATCH v5 1/3] common/sfc_efx/base: report VLAN stripping capability Artemii Morozov
2023-06-19 9:43 ` Andrew Rybchenko
2023-06-13 15:12 ` [PATCH v5 2/3] common/sfc_efx/base: add support to enable VLAN stripping Artemii Morozov
2023-06-19 10:28 ` Andrew Rybchenko
2023-06-20 9:55 ` Artemii Morozov
2023-06-20 11:50 ` Andrew Rybchenko
2023-06-20 13:10 ` Artemii Morozov
2023-06-20 13:53 ` Andrew Rybchenko
2023-06-13 15:12 ` [PATCH v5 3/3] net/sfc: support VLAN stripping offload Artemii Morozov
2023-06-19 10:36 ` Andrew Rybchenko
2023-06-22 11:31 ` [PATCH v6 0/4] " Artemii Morozov
2023-06-22 11:31 ` [PATCH v6 1/4] common/sfc_efx/base: report VLAN stripping capability Artemii Morozov
2023-06-22 11:46 ` Andrew Rybchenko
2023-06-22 11:31 ` [PATCH v6 2/4] common/sfc_efx/base: add API to get installed filters count Artemii Morozov
2023-06-22 11:51 ` Andrew Rybchenko
2023-06-22 11:31 ` [PATCH v6 3/4] common/sfc_efx/base: add support to enable VLAN stripping Artemii Morozov
2023-06-22 11:54 ` Andrew Rybchenko
2023-06-22 11:31 ` [PATCH v6 4/4] net/sfc: support VLAN stripping offload Artemii Morozov
2023-06-22 12:07 ` Andrew Rybchenko
2023-06-22 15:11 ` [PATCH v7 0/4] " Artemii Morozov
2023-06-22 15:11 ` [PATCH v7 1/4] common/sfc_efx/base: report VLAN stripping capability Artemii Morozov
2023-06-22 15:11 ` [PATCH v7 2/4] common/sfc_efx/base: add API to get installed filters count Artemii Morozov
2023-06-22 15:42 ` Andrew Rybchenko
2023-06-22 15:11 ` [PATCH v7 3/4] common/sfc_efx/base: add support to enable VLAN stripping Artemii Morozov
2023-06-22 15:40 ` Andrew Rybchenko
2023-06-22 15:11 ` [PATCH v7 4/4] net/sfc: support VLAN stripping offload Artemii Morozov
2023-06-22 15:41 ` Andrew Rybchenko
2023-06-22 16:05 ` Ferruh Yigit
2023-06-23 5:47 ` [PATCH v8 0/4] " Artemii Morozov
2023-06-23 5:47 ` [PATCH v8 1/4] common/sfc_efx/base: report VLAN stripping capability Artemii Morozov
2023-06-23 5:47 ` [PATCH v8 2/4] common/sfc_efx/base: add API to get installed filters count Artemii Morozov
2023-06-23 7:24 ` Ivan Malov
2023-06-23 5:47 ` [PATCH v8 3/4] common/sfc_efx/base: add support to enable VLAN stripping Artemii Morozov
2023-06-23 5:47 ` [PATCH v8 4/4] net/sfc: support VLAN stripping offload Artemii Morozov
2023-06-23 12:35 ` [PATCH v8 0/4] " Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c4d72eb1-86a3-b797-9f6b-c2ae485c958b@oktetlabs.ru \
--to=andrew.rybchenko@oktetlabs.ru \
--cc=amoreton@xilinx.com \
--cc=artemii.morozov@arknetworks.am \
--cc=dev@dpdk.org \
--cc=ivan.malov@arknetworks.am \
--cc=viacheslav.galaktionov@arknetworks.am \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).