[ovs-dev,v2,5/5] dp-packet: Use memcpy on dp_packet elements.
diff mbox

Message ID 1500480297-7530-5-git-send-email-antonio.fischetti@intel.com
State Superseded
Delegated to: Darrell Ball
Headers show

Commit Message

Fischetti, Antonio July 19, 2017, 4:04 p.m. UTC
memcpy replaces the several single copies inside
dp_packet_clone_with_headroom().

Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 lib/dp-packet.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Billy O'Mahony Aug. 1, 2017, 2:14 p.m. UTC | #1
Hi Antonio,

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of antonio.fischetti@intel.com
> Sent: Wednesday, July 19, 2017 5:05 PM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH v2 5/5] dp-packet: Use memcpy on dp_packet
> elements.
> 
> memcpy replaces the several single copies inside
> dp_packet_clone_with_headroom().
> 
> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> ---
>  lib/dp-packet.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 67aa406..f4dbcb7 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -157,8 +157,9 @@ dp_packet_clone(const struct dp_packet *buffer)
>      return dp_packet_clone_with_headroom(buffer, 0);  }
> 
> -/* Creates and returns a new dp_packet whose data are copied from
> 'buffer'.   The
> - * returned dp_packet will additionally have 'headroom' bytes of headroom.
> */
> +/* Creates and returns a new dp_packet whose data are copied from
> 'buffer'.
> + * The returned dp_packet will additionally have 'headroom' bytes of
> + * headroom. */
>  struct dp_packet *
>  dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t
> headroom)  { @@ -167,13 +168,12 @@
> dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t
> headroom)
>      new_buffer =
> dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
>                                                   dp_packet_size(buffer),
>                                                   headroom);
> -    new_buffer->l2_pad_size = buffer->l2_pad_size;
> -    new_buffer->l2_5_ofs = buffer->l2_5_ofs;
> -    new_buffer->l3_ofs = buffer->l3_ofs;
> -    new_buffer->l4_ofs = buffer->l4_ofs;
> -    new_buffer->md = buffer->md;
> -    new_buffer->cutlen = buffer->cutlen;
> -    new_buffer->packet_type = buffer->packet_type;
> +    /* Copy the following fields into the returned buffer: l2_pad_size,
> +     * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
> +    memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size,
> +            sizeof(struct dp_packet) -
> +            offsetof(struct dp_packet, l2_pad_size));
> +
[[BO'M]] 
Does this change in itself have a perf improvement?

A reference/warning in the dp_packet declaration would be a good idea -  anyone changing fields from l2_pad_size to the end of the structure needs to be aware of this implementation of dp_packet_clone_data_with_headroom().

>  #ifdef DPDK_NETDEV
>      new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;  #else
> --
> 2.4.11
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Fischetti, Antonio Aug. 3, 2017, 9:06 a.m. UTC | #2
> -----Original Message-----
> From: O Mahony, Billy
> Sent: Tuesday, August 1, 2017 3:15 PM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: RE: [ovs-dev] [PATCH v2 5/5] dp-packet: Use memcpy on dp_packet
> elements.
> 
> Hi Antonio,
> 
> > -----Original Message-----
> > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> > bounces@openvswitch.org] On Behalf Of antonio.fischetti@intel.com
> > Sent: Wednesday, July 19, 2017 5:05 PM
> > To: dev@openvswitch.org
> > Subject: [ovs-dev] [PATCH v2 5/5] dp-packet: Use memcpy on dp_packet
> > elements.
> >
> > memcpy replaces the several single copies inside
> > dp_packet_clone_with_headroom().
> >
> > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> > ---
> >  lib/dp-packet.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 67aa406..f4dbcb7 100644
> > --- a/lib/dp-packet.c
> > +++ b/lib/dp-packet.c
> > @@ -157,8 +157,9 @@ dp_packet_clone(const struct dp_packet *buffer)
> >      return dp_packet_clone_with_headroom(buffer, 0);  }
> >
> > -/* Creates and returns a new dp_packet whose data are copied from
> > 'buffer'.   The
> > - * returned dp_packet will additionally have 'headroom' bytes of headroom.
> > */
> > +/* Creates and returns a new dp_packet whose data are copied from
> > 'buffer'.
> > + * The returned dp_packet will additionally have 'headroom' bytes of
> > + * headroom. */
> >  struct dp_packet *
> >  dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t
> > headroom)  { @@ -167,13 +168,12 @@
> > dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t
> > headroom)
> >      new_buffer =
> > dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
> >                                                   dp_packet_size(buffer),
> >                                                   headroom);
> > -    new_buffer->l2_pad_size = buffer->l2_pad_size;
> > -    new_buffer->l2_5_ofs = buffer->l2_5_ofs;
> > -    new_buffer->l3_ofs = buffer->l3_ofs;
> > -    new_buffer->l4_ofs = buffer->l4_ofs;
> > -    new_buffer->md = buffer->md;
> > -    new_buffer->cutlen = buffer->cutlen;
> > -    new_buffer->packet_type = buffer->packet_type;
> > +    /* Copy the following fields into the returned buffer: l2_pad_size,
> > +     * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
> > +    memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size,
> > +            sizeof(struct dp_packet) -
> > +            offsetof(struct dp_packet, l2_pad_size));
> > +
> [[BO'M]]
> Does this change in itself have a perf improvement?

[Antonio]
I tested this change by comparing the CPU Time over a 60 sec analysis
with VTune.

In original ovs:
dp_packet_clone_with_headroom    4.530s

+ this changes:
dp_packet_clone_with_headroom    3.920s

Further details were reported in this reply for v1
https://mail.openvswitch.org/pipermail/ovs-dev/2017-June/334536.html )


> 
> A reference/warning in the dp_packet declaration would be a good idea -  anyone
> changing fields from l2_pad_size to the end of the structure needs to be aware
> of this implementation of dp_packet_clone_data_with_headroom().

[Antonio] agree, will do that.


> 
> >  #ifdef DPDK_NETDEV
> >      new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;  #else
> > --
> > 2.4.11
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch
diff mbox

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 67aa406..f4dbcb7 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -157,8 +157,9 @@  dp_packet_clone(const struct dp_packet *buffer)
     return dp_packet_clone_with_headroom(buffer, 0);
 }
 
-/* Creates and returns a new dp_packet whose data are copied from 'buffer'.   The
- * returned dp_packet will additionally have 'headroom' bytes of headroom. */
+/* Creates and returns a new dp_packet whose data are copied from 'buffer'.
+ * The returned dp_packet will additionally have 'headroom' bytes of
+ * headroom. */
 struct dp_packet *
 dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom)
 {
@@ -167,13 +168,12 @@  dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom)
     new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
                                                  dp_packet_size(buffer),
                                                  headroom);
-    new_buffer->l2_pad_size = buffer->l2_pad_size;
-    new_buffer->l2_5_ofs = buffer->l2_5_ofs;
-    new_buffer->l3_ofs = buffer->l3_ofs;
-    new_buffer->l4_ofs = buffer->l4_ofs;
-    new_buffer->md = buffer->md;
-    new_buffer->cutlen = buffer->cutlen;
-    new_buffer->packet_type = buffer->packet_type;
+    /* Copy the following fields into the returned buffer: l2_pad_size,
+     * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
+    memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size,
+            sizeof(struct dp_packet) -
+            offsetof(struct dp_packet, l2_pad_size));
+
 #ifdef DPDK_NETDEV
     new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
 #else