[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