RE: [PATCH v2 05/10] Optimize the function of packet_new

[Rao, Lei]

How about redefine a function named packet_new_nocopy? 
In comments, we can tell the caller don't release the buffer and the 
packet_destroy will release it.

Thanks,
Lei.
-----Original Message-----
From: lizhijian@fujitsu.com <lizhijian@fujitsu.com> 
Sent: Friday, March 12, 2021 2:53 PM
To: Rao, Lei <lei.rao@intel.com>; Zhang, Chen <chen.zhang@intel.com>; 
jasowang@redhat.com; quintela@redhat.com; dgilbert@redhat.com; 
pbonzini@redhat.com; lukasstraub2@web.de
Cc: qemu-devel@nongnu.org
Subject: Re: [PATCH v2 05/10] Optimize the function of packet_new



On 3/12/21 1:02 PM, leirao wrote:
> From: "Rao, Lei" <lei.rao@intel.com>
>
> if we put the data copy outside the packet_new(), then for the 
> filter-rewrite module, there will be one less memory copy in the 
> processing of each network packet.
>
> Signed-off-by: Lei Rao <lei.rao@intel.com>
> ---
>   net/colo-compare.c    | 7 +++++--
>   net/colo.c            | 4 ++--
>   net/colo.h            | 2 +-
>   net/filter-rewriter.c | 1 -
>   4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c index 
> 9e18baa..8bdf5a8 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -247,14 +247,17 @@ static int packet_enqueue(CompareState *s, int mode, 
> Connection **con)
>       ConnectionKey key;
>       Packet *pkt = NULL;
>       Connection *conn;
> +    char *data = NULL;
>       int ret;
>   
>       if (mode == PRIMARY_IN) {
> -        pkt = packet_new(s->pri_rs.buf,
> +        data = g_memdup(s->pri_rs.buf, s->pri_rs.packet_len);
> +        pkt = packet_new(data,
>                            s->pri_rs.packet_len,
>                            s->pri_rs.vnet_hdr_len);
>       } else {
> -        pkt = packet_new(s->sec_rs.buf,
> +        data = g_memdup(s->sec_rs.buf, s->sec_rs.packet_len);
> +        pkt = packet_new(data,
>                            s->sec_rs.packet_len,
>                            s->sec_rs.vnet_hdr_len);
>       }
> diff --git a/net/colo.c b/net/colo.c
> index ef00609..08fb37e 100644
> --- a/net/colo.c
> +++ b/net/colo.c
> @@ -155,11 +155,11 @@ void connection_destroy(void *opaque)
>       g_slice_free(Connection, conn);
>   }
>   
> -Packet *packet_new(const void *data, int size, int vnet_hdr_len)
> +Packet *packet_new(void *data, int size, int vnet_hdr_len)
>   {
>       Packet *pkt = g_slice_new(Packet);
>   
> -    pkt->data = g_memdup(data, size);
> +    pkt->data = data;

if so,  should packet_destroy()  free() data which may be not alloc by itself

Thanks
Zhijian