On Thu, Oct 9, 2014 at 10:57 AM, Jarno Rajahalme <[email protected]> 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 <[email protected]> > > > commit 502f8c94c11e1994b5f4a6e73aa865ee29642b14 > Author: Jarno Rajahalme <[email protected]> > 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 <[email protected]> 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 <[email protected]> >> --- >> 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 >> [email protected] >> http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
