[v3,2/5] mbuf: add second dynamic field member for VA only build
Checks
Commit Message
mbuf physical address field is not used in builds which only uses VA. It
is used to expand the dynamic field area.
Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
lib/mbuf/rte_mbuf_core.h | 26 +++++++++++++++++---------
lib/mbuf/rte_mbuf_dyn.c | 2 ++
2 files changed, 19 insertions(+), 9 deletions(-)
Comments
21/09/2022 15:56, Shijith Thotton:
> mbuf physical address field is not used in builds which only uses VA. It
> is used to expand the dynamic field area.
>
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
We cannot condition the use of the dynamic field.
I think it is enough justification to reject this patch.
And about adding a compilation option for IOVA in the first patch of this series,
I think it is not the direction the majority wants DPDK to go.
We tend to avoid compilation options.
> @@ -579,15 +579,23 @@ struct rte_mbuf {
> RTE_MARKER cacheline0;
>
> void *buf_addr; /**< Virtual address of segment buffer. */
> - /**
> - * Physical address of segment buffer.
> - * This field is invalid if the build is configured to use only
> - * virtual address as IOVA (i.e. RTE_IOVA_AS_VA is 1).
> - * Force alignment to 8-bytes, so as to ensure we have the exact
> - * same mbuf cacheline0 layout for 32-bit and 64-bit. This makes
> - * working on vector drivers easier.
> - */
> - rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
> + RTE_STD_C11
> + union {
> + /**
> + * Physical address of segment buffer.
> + * This field is invalid if the build is configured to use only
> + * virtual address as IOVA (i.e. RTE_IOVA_AS_VA is 1).
> + * Force alignment to 8-bytes, so as to ensure we have the exact
> + * same mbuf cacheline0 layout for 32-bit and 64-bit. This makes
> + * working on vector drivers easier.
> + */
> + rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
> + /**
> + * Reserved for dynamic field in builds where physical address
> + * field is invalid.
> + */
> + uint64_t dynfield2;
> + };
Hi,
On Wed, Sep 21, 2022 at 07:26:18PM +0530, Shijith Thotton wrote:
> mbuf physical address field is not used in builds which only uses VA. It
> is used to expand the dynamic field area.
>
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---
> lib/mbuf/rte_mbuf_core.h | 26 +++++++++++++++++---------
> lib/mbuf/rte_mbuf_dyn.c | 2 ++
> 2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> index c6292e7252..94907f301d 100644
> --- a/lib/mbuf/rte_mbuf_core.h
> +++ b/lib/mbuf/rte_mbuf_core.h
> @@ -579,15 +579,23 @@ struct rte_mbuf {
> RTE_MARKER cacheline0;
>
> void *buf_addr; /**< Virtual address of segment buffer. */
> - /**
> - * Physical address of segment buffer.
> - * This field is invalid if the build is configured to use only
> - * virtual address as IOVA (i.e. RTE_IOVA_AS_VA is 1).
> - * Force alignment to 8-bytes, so as to ensure we have the exact
> - * same mbuf cacheline0 layout for 32-bit and 64-bit. This makes
> - * working on vector drivers easier.
> - */
> - rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
> + RTE_STD_C11
> + union {
> + /**
> + * Physical address of segment buffer.
> + * This field is invalid if the build is configured to use only
> + * virtual address as IOVA (i.e. RTE_IOVA_AS_VA is 1).
> + * Force alignment to 8-bytes, so as to ensure we have the exact
> + * same mbuf cacheline0 layout for 32-bit and 64-bit. This makes
> + * working on vector drivers easier.
> + */
> + rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
> + /**
> + * Reserved for dynamic field in builds where physical address
> + * field is invalid.
> + */
> + uint64_t dynfield2;
> + };
Same comment than on previous patch: using a #if instead of the union here looks
better to me to ensure that we never use buf_iova when RTE_IOVA_AS_VA=1.
>
> /* next 8 bytes are initialised on RX descriptor rearm */
> RTE_MARKER64 rearm_data;
> diff --git a/lib/mbuf/rte_mbuf_dyn.c b/lib/mbuf/rte_mbuf_dyn.c
> index 4ae79383b5..6a4cf96897 100644
> --- a/lib/mbuf/rte_mbuf_dyn.c
> +++ b/lib/mbuf/rte_mbuf_dyn.c
> @@ -128,6 +128,8 @@ init_shared_mem(void)
> */
> memset(shm, 0, sizeof(*shm));
> mark_free(dynfield1);
> + if (RTE_IOVA_AS_VA)
> + mark_free(dynfield2);
In this case, it will have to be a #if here too.
>
> /* init free_flags */
> for (mask = RTE_MBUF_F_FIRST_FREE; mask <= RTE_MBUF_F_LAST_FREE; mask <<= 1)
Also, I think we can add in the RTE_IOVA_AS_VA documentation that it
replaces the buf_iova by 8 bytes of dynamic field.
On Wed, Sep 28, 2022 at 09:24:51AM +0200, Thomas Monjalon wrote:
> 21/09/2022 15:56, Shijith Thotton:
> > mbuf physical address field is not used in builds which only uses VA. It
> > is used to expand the dynamic field area.
> >
> > Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>
> We cannot condition the use of the dynamic field.
> I think it is enough justification to reject this patch.
I don't think it is an issue.
> And about adding a compilation option for IOVA in the first patch of this series,
> I think it is not the direction the majority wants DPDK to go.
> We tend to avoid compilation options.
In general, I agree that we don't want to have many custom compile-time options,
especially if they impact ABI. It has several issues that have already been
widely discussed.
However, in this specific case, we can suppose that removing buf_iova is a
long-term goal (in years). Having this compile-time option is a way to test this
approach, and progressively prepare the drivers to support it. Then, in few
years (if we are still convinced), we may announce an abi breakage and switch to
this new mode by default.
Olivier
28/09/2022 14:52, Olivier Matz:
> On Wed, Sep 28, 2022 at 09:24:51AM +0200, Thomas Monjalon wrote:
> > 21/09/2022 15:56, Shijith Thotton:
> > > mbuf physical address field is not used in builds which only uses VA. It
> > > is used to expand the dynamic field area.
> > >
> > > Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> >
> > We cannot condition the use of the dynamic field.
> > I think it is enough justification to reject this patch.
>
> I don't think it is an issue.
>
> > And about adding a compilation option for IOVA in the first patch of this series,
> > I think it is not the direction the majority wants DPDK to go.
> > We tend to avoid compilation options.
>
> In general, I agree that we don't want to have many custom compile-time options,
> especially if they impact ABI. It has several issues that have already been
> widely discussed.
>
> However, in this specific case, we can suppose that removing buf_iova is a
> long-term goal (in years). Having this compile-time option is a way to test this
> approach, and progressively prepare the drivers to support it. Then, in few
> years (if we are still convinced), we may announce an abi breakage and switch to
> this new mode by default.
You convinced me.
On Wed, 28 Sep 2022 14:52:47 +0200
Olivier Matz <olivier.matz@6wind.com> wrote:
> On Wed, Sep 28, 2022 at 09:24:51AM +0200, Thomas Monjalon wrote:
> > 21/09/2022 15:56, Shijith Thotton:
> > > mbuf physical address field is not used in builds which only uses VA. It
> > > is used to expand the dynamic field area.
> > >
> > > Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> >
> > We cannot condition the use of the dynamic field.
> > I think it is enough justification to reject this patch.
>
> I don't think it is an issue.
>
> > And about adding a compilation option for IOVA in the first patch of this series,
> > I think it is not the direction the majority wants DPDK to go.
> > We tend to avoid compilation options.
>
> In general, I agree that we don't want to have many custom compile-time options,
> especially if they impact ABI. It has several issues that have already been
> widely discussed.
>
> However, in this specific case, we can suppose that removing buf_iova is a
> long-term goal (in years). Having this compile-time option is a way to test this
> approach, and progressively prepare the drivers to support it. Then, in few
> years (if we are still convinced), we may announce an abi breakage and switch to
> this new mode by default.
Since field is invalid if compile option is set,
shouldn't the field be inside an #ifdef so that if a driver or application
was to make the mistake of using that directly, it would fail at compile
time instead of runtime.
Leaving booby traps for applications and drivers is bad design.
>> > > mbuf physical address field is not used in builds which only uses VA. It
>> > > is used to expand the dynamic field area.
>> > >
>> > > Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>> >
>> > We cannot condition the use of the dynamic field.
>> > I think it is enough justification to reject this patch.
>>
>> I don't think it is an issue.
>>
>> > And about adding a compilation option for IOVA in the first patch of this series,
>> > I think it is not the direction the majority wants DPDK to go.
>> > We tend to avoid compilation options.
>>
>> In general, I agree that we don't want to have many custom compile-time
>options,
>> especially if they impact ABI. It has several issues that have already been
>> widely discussed.
>>
>> However, in this specific case, we can suppose that removing buf_iova is a
>> long-term goal (in years). Having this compile-time option is a way to test this
>> approach, and progressively prepare the drivers to support it. Then, in few
>> years (if we are still convinced), we may announce an abi breakage and switch to
>> this new mode by default.
>
>Since field is invalid if compile option is set,
>shouldn't the field be inside an #ifdef so that if a driver or application
>was to make the mistake of using that directly, it would fail at compile
>time instead of runtime.
>
>Leaving booby traps for applications and drivers is bad design.
Will move to using #ifdef in v4.
@@ -579,15 +579,23 @@ struct rte_mbuf {
RTE_MARKER cacheline0;
void *buf_addr; /**< Virtual address of segment buffer. */
- /**
- * Physical address of segment buffer.
- * This field is invalid if the build is configured to use only
- * virtual address as IOVA (i.e. RTE_IOVA_AS_VA is 1).
- * Force alignment to 8-bytes, so as to ensure we have the exact
- * same mbuf cacheline0 layout for 32-bit and 64-bit. This makes
- * working on vector drivers easier.
- */
- rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
+ RTE_STD_C11
+ union {
+ /**
+ * Physical address of segment buffer.
+ * This field is invalid if the build is configured to use only
+ * virtual address as IOVA (i.e. RTE_IOVA_AS_VA is 1).
+ * Force alignment to 8-bytes, so as to ensure we have the exact
+ * same mbuf cacheline0 layout for 32-bit and 64-bit. This makes
+ * working on vector drivers easier.
+ */
+ rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
+ /**
+ * Reserved for dynamic field in builds where physical address
+ * field is invalid.
+ */
+ uint64_t dynfield2;
+ };
/* next 8 bytes are initialised on RX descriptor rearm */
RTE_MARKER64 rearm_data;
@@ -128,6 +128,8 @@ init_shared_mem(void)
*/
memset(shm, 0, sizeof(*shm));
mark_free(dynfield1);
+ if (RTE_IOVA_AS_VA)
+ mark_free(dynfield2);
/* init free_flags */
for (mask = RTE_MBUF_F_FIRST_FREE; mask <= RTE_MBUF_F_LAST_FREE; mask <<= 1)