[ovs-dev] [PATCH RFC 1/2] netdev-dpif: Add metadata to dpif-packet.

Pravin Shelar pshelar at nicira.com
Thu Oct 9 20:53:34 UTC 2014


On Thu, Oct 9, 2014 at 10:57 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> Pravin,
>
> This patch did not work by itself, I applied the following incremental (some of which are purely cosmetically, though) to make this pass the unit tests. Also, please find some further comments below.
>
I did make check but with default configure option did not catch any
error. Your changes looks good I have folded it the patch. I will push
this patch after few minutes.

> With these:
>
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
>
>
> commit 502f8c94c11e1994b5f4a6e73aa865ee29642b14
> Author: Jarno Rajahalme <jrajahalme at nicira.com>
> Date:   Thu Oct 9 10:14:09 2014 -0700
>
>     Fixups.
> ---
>  lib/dpif-netdev.c            |    4 +++-
>  lib/dpif.c                   |    1 +
>  lib/odp-execute.c            |    5 +----
>  ofproto/ofproto-dpif-xlate.c |    2 --
>  4 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index ba217c5..40ed0f3 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1860,6 +1860,7 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
>      }
>
>      packet.ofpbuf = *execute->packet;
> +    packet.md = execute->md;
>      pp = &packet;
>
>      /* Tries finding the 'pmd'.  If NULL is returned, that means
> @@ -1885,6 +1886,7 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
>       * reallocate the ofpbuf memory. We need to pass those changes to the
>       * caller */
>      *execute->packet = packet.ofpbuf;
> +    execute->md = packet.md;
>
>      return 0;
>  }
> @@ -2922,7 +2924,7 @@ dp_netdev_execute_actions(struct dp_netdev_pmd_thread *pmd,
>                            bool may_steal,
>                            const struct nlattr *actions, size_t actions_len)
>  {
> -    struct dp_netdev_execute_aux aux = {pmd};
> +    struct dp_netdev_execute_aux aux = { pmd };
>
>      odp_execute_actions(&aux, packets, cnt, may_steal, actions,
>                          actions_len, dp_execute_cb);
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 34d8c13..d088f68 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1074,6 +1074,7 @@ dpif_execute_with_help(struct dpif *dpif, struct dpif_execute *execute)
>       * reallocate the ofpbuf memory. We need to pass those changes to the
>       * caller */
>      *execute->packet = packet.ofpbuf;
> +    execute->md = packet.md;
>
>      return aux.error;
>  }
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index b179cf2..230e6e1 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -441,10 +441,7 @@ odp_execute_actions(void *dp, struct dpif_packet **packets, int cnt, bool steal,
>                  uint32_t hash;
>
>                  for (i = 0; i < cnt; i++) {
> -                    struct ofpbuf *buf = &packets[i]->ofpbuf;
> -                    struct pkt_metadata *md = &packets[i]->md;
> -
> -                    flow_extract(buf, md, &flow);
> +                    flow_extract(&packets[i]->ofpbuf, &packets[i]->md, &flow);
>                      hash = flow_hash_5tuple(&flow, hash_act->hash_basis);
>
>                      /* We also store the hash value with each packet */
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index cc14b31..07a1f29 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3015,7 +3015,6 @@ execute_controller_action(struct xlate_ctx *ctx, int len,
>  {
>      struct ofproto_packet_in *pin;
>      struct dpif_packet *packet;
> -    struct pkt_metadata md = PKT_METADATA_INITIALIZER(0);
>
>      ctx->xout->slow |= SLOW_CONTROLLER;
>      if (!ctx->xin->packet) {
> @@ -3029,7 +3028,6 @@ execute_controller_action(struct xlate_ctx *ctx, int len,
>                                            &ctx->xout->wc,
>                                            ctx->xbridge->masked_set_action);
>
> -    packet->md = md;
>      odp_execute_actions(NULL, &packet, 1, false,
>                          ofpbuf_data(ctx->xout->odp_actions),
>                          ofpbuf_size(ctx->xout->odp_actions), NULL);
>
>
> On Oct 3, 2014, at 8:23 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
>
>> Today dpif-netdev has single metadat for given batch, since one
>> batch belongs to one port, but soon packets fro single tunnel ports
>> can belong to different ports, so we need to have per packet metadata.
>>
>> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>> ---
>> lib/dpif-netdev.c            | 68 ++++++++++++++++++++------------------------
>> lib/dpif.c                   |  7 +++--
>> lib/odp-execute.c            | 28 ++++++++----------
>> lib/odp-execute.h            |  3 +-
>> lib/packet-dpif.c            |  3 ++
>> lib/packet-dpif.h            |  1 +
>> ofproto/ofproto-dpif-xlate.c |  3 +-
>> 7 files changed, 54 insertions(+), 59 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 9c61a21..a7c9c92 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -391,12 +391,12 @@ static int dpif_netdev_open(const struct dpif_class *, const char *name,
>>                             bool create, struct dpif **);
>> static void dp_netdev_execute_actions(struct dp_netdev_pmd_thread *pmd,
>>                                       struct dpif_packet **, int c,
>> -                                      bool may_steal, struct pkt_metadata *,
>> +                                      bool may_steal,
>>                                       const struct nlattr *actions,
>>                                       size_t actions_len);
>> static void dp_netdev_input(struct dp_netdev_pmd_thread *,
>> -                            struct dpif_packet **, int cnt,
>> -                            struct pkt_metadata *);
>> +                            struct dpif_packet **, int cnt);
>> +
>> static void dp_netdev_disable_upcall(struct dp_netdev *);
>> static void dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd,
>>                                     struct dp_netdev *dp, int index,
>> @@ -1860,7 +1860,6 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
>>     struct dp_netdev *dp = get_dp_netdev(dpif);
>>     struct dp_netdev_pmd_thread *pmd;
>>     struct dpif_packet packet, *pp;
>> -    struct pkt_metadata *md = &execute->md;
>>
>>     if (ofpbuf_size(execute->packet) < ETH_HEADER_LEN ||
>>         ofpbuf_size(execute->packet) > UINT16_MAX) {
>> @@ -1883,7 +1882,7 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
>>     if (pmd->core_id == NON_PMD_CORE_ID) {
>>         ovs_mutex_lock(&dp->non_pmd_mutex);
>>     }
>> -    dp_netdev_execute_actions(pmd, &pp, 1, false, md, execute->actions,
>> +    dp_netdev_execute_actions(pmd, &pp, 1, false, execute->actions,
>>                               execute->actions_len);
>>     if (pmd->core_id == NON_PMD_CORE_ID) {
>>         ovs_mutex_unlock(&dp->non_pmd_mutex);
>> @@ -2044,10 +2043,15 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread *pmd,
>>
>>     error = netdev_rxq_recv(rxq, packets, &cnt);
>>     if (!error) {
>> -        struct pkt_metadata md = PKT_METADATA_INITIALIZER(port->port_no);
>> +        int i;
>>
>>         *recirc_depth_get() = 0;
>> -        dp_netdev_input(pmd, packets, cnt, &md);
>> +
>> +        /* XXX: initialize md in netdev implementation. */
>
> Please do :-)
>
This involves pushing odp port info to netdev, plus it involves
creating per dpdk-dev pool to do it efficiently. thats why I postponed
this change for later.

>> +        for (i = 0; i < cnt; i++) {
>> +            packets[i]->md = PKT_METADATA_INITIALIZER(port->port_no);
>> +        }
>> +        dp_netdev_input(pmd, packets, cnt);
>>     } else if (error != EAGAIN && error != EOPNOTSUPP) {
>>         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>>
>> @@ -2493,7 +2497,6 @@ struct packet_batch {
>>     struct dp_netdev_flow *flow;
>>
>>     struct dpif_packet *packets[NETDEV_MAX_RX_BATCH];
>> -    struct pkt_metadata md;
>> };
>>
>
> (snip)
>
>> diff --git a/lib/packet-dpif.c b/lib/packet-dpif.c
>> index 3a912e1..5b6e1dc 100644
>> --- a/lib/packet-dpif.c
>> +++ b/lib/packet-dpif.c
>> @@ -27,6 +27,7 @@ dpif_packet_new_with_headroom(size_t size, size_t headroom)
>>
>>     ofpbuf_init(b, size + headroom);
>>     ofpbuf_reserve(b, headroom);
>> +    memset(&p->md, 0, sizeof p->md);
>>
>>     return p;
>> }
>> @@ -38,6 +39,7 @@ dpif_packet_clone_from_ofpbuf(const struct ofpbuf *b)
>>     size_t headroom = ofpbuf_headroom(b);
>>
>>     ofpbuf_init(&p->ofpbuf, ofpbuf_size(b) + headroom);
>> +    memset(&p->md, 0, sizeof p->md);
>>     ofpbuf_reserve(&p->ofpbuf, headroom);
>>
>>     ofpbuf_put(&p->ofpbuf, ofpbuf_data(b), ofpbuf_size(b));
>> @@ -61,6 +63,7 @@ dpif_packet_clone(struct dpif_packet *p)
>>     struct dpif_packet *newp;
>>
>>     newp = dpif_packet_clone_from_ofpbuf(&p->ofpbuf);
>> +    memcpy(&newp->md, &p->md, sizeof p->md);
>>
>>     dpif_packet_set_dp_hash(newp, dpif_packet_get_dp_hash(p));
>>
>> diff --git a/lib/packet-dpif.h b/lib/packet-dpif.h
>> index 89f048e..1a5efb6 100644
>> --- a/lib/packet-dpif.h
>> +++ b/lib/packet-dpif.h
>> @@ -30,6 +30,7 @@ struct dpif_packet {
>> #ifndef DPDK_NETDEV
>>     uint32_t dp_hash;           /* Packet hash. */
>> #endif
>> +    struct pkt_metadata md;
>> };
>
> Now that the full metadata is in struct dpif_packet, we should remore the conditional dp_hash member and use the md.dp_hash instead.
>
This changes flow key for first packet where currently hash is zero. I
did not wanted to introduce any functionality related change in this
patch.

> It would also be good to use struct dpif_packet in struct execute directly. That way we would avoid copying the ofpbuf and md around in both dpif_execute_with_help() and dpif_netdev_execute().
>
Yes, This will be nice.
Both changes looks like optimization, I will do it follow on patch.

Thanks for review.

>>
>> struct dpif_packet *dpif_packet_new_with_headroom(size_t size,
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 1d46456..880824d 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -3029,7 +3029,8 @@ execute_controller_action(struct xlate_ctx *ctx, int len,
>>                                           &ctx->xout->wc,
>>                                           ctx->xbridge->masked_set_action);
>>
>> -    odp_execute_actions(NULL, &packet, 1, false, &md,
>> +    packet->md = md;
>> +    odp_execute_actions(NULL, &packet, 1, false,
>>                         ofpbuf_data(ctx->xout->odp_actions),
>>                         ofpbuf_size(ctx->xout->odp_actions), NULL);
>>
>> --
>> 1.9.3
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>



More information about the dev mailing list