I think this is on the right track, just some minor comments.
You aren't planning to have anyone call xlate_actions_unsafe() in
future patches right? If that's true I'd rather keep the public API
the same and do the safe/unsafe split internally. I.E:
Rename xlate_actions_safe() to xlate_actions().
Remove xlate_actions_unsafe() entirely, and simply call
xlate_actions__() directly when necessary. Alternatively you could
rename xlate_actions__() xlate_actions_unsafe() instead, that's a
matter of personal preference.
I doubt it matters much in practice, but I'd prefer we release the
xlate_rwlock before we call dpif_execute() in case that takes a long
time. You don't need the xlate_rwlock to call xlate_out_uninit(), so
it should be a simple matter of moving the call before the
dpif_execute() call.
Along those same lines, there's a lot of working setting up the xin
which could be pulled out of the critical section. We could do the
flow_extract() beforehand for example. Also initializing the the
output ofpact.
Are you planning to shove a lock around ofproto->stats? Updating it
in ofproto_dpif_send_packet() isn't thread safe.
Thanks,
Ethan
On Fri, Sep 27, 2013 at 3:39 PM, Alex Wang <al...@nicira.com> wrote:
> This commit moves the main logic of send_packet() function into
> the ofproto-dpif-xlate module. Also, the xlate_actions() function
> in ofproto-dpif-xlate module is adjusted to guarantee thread safety.
>
> Signed-off-by: Alex Wang <al...@nicira.com>
> ---
>
> v1 -> v2:
> - change xlate_actions to a thread safe version and thread unsafe version
>
> ---
> ofproto/ofproto-dpif-upcall.c | 4 +-
> ofproto/ofproto-dpif-xlate.c | 85
> +++++++++++++++++++++++++++++++++++++----
> ofproto/ofproto-dpif-xlate.h | 5 ++-
> ofproto/ofproto-dpif.c | 71 ++++++++--------------------------
> ofproto/ofproto-dpif.h | 1 +
> 5 files changed, 101 insertions(+), 65 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 180b87e..33c75b7 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -230,7 +230,7 @@ udpif_wait(struct udpif *udpif)
> }
>
> /* Notifies 'udpif' that something changed which may render previous
> - * xlate_actions() results invalid. */
> + * xlate_actions_safe() results invalid. */
> void
> udpif_revalidate(struct udpif *udpif)
> {
> @@ -731,7 +731,7 @@ handle_miss_upcalls(struct udpif *udpif, struct list
> *upcalls)
> miss->stats.tcp_flags, NULL);
> xin.may_learn = true;
> xin.resubmit_stats = &miss->stats;
> - xlate_actions(&xin, &miss->xout);
> + xlate_actions_safe(&xin, &miss->xout);
> flow_wildcards_or(&miss->xout.wc, &miss->xout.wc, &wc);
> rule_dpif_unref(rule);
> }
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a5b6814..020588f 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -192,6 +192,8 @@ static struct hmap xports = HMAP_INITIALIZER(&xports);
> static bool may_receive(const struct xport *, struct xlate_ctx *);
> static void do_xlate_actions(const struct ofpact *, size_t ofpacts_len,
> struct xlate_ctx *);
> +static void xlate_actions__(struct xlate_in *, struct xlate_out *)
> + OVS_REQ_RDLOCK(xlate_rwlock);
> static void xlate_normal(struct xlate_ctx *);
> static void xlate_report(struct xlate_ctx *, const char *);
> static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port,
> @@ -2434,7 +2436,7 @@ xlate_actions_for_side_effects(struct xlate_in *xin)
> {
> struct xlate_out xout;
>
> - xlate_actions(xin, &xout);
> + xlate_actions_safe(xin, &xout);
> xlate_out_uninit(&xout);
> }
>
> @@ -2515,13 +2517,31 @@ actions_output_to_local_port(const struct xlate_ctx
> *ctx)
> return false;
> }
>
> +/* Thread safe call to xlate_actions__(). */
> +void
> +xlate_actions_safe(struct xlate_in *xin, struct xlate_out *xout)
> +{
> + ovs_rwlock_rdlock(&xlate_rwlock);
> + xlate_actions__(xin, xout);
> + ovs_rwlock_unlock(&xlate_rwlock);
> +}
> +
> +/* Thread unsafe call to xlate_actions__().
> + * Caller must acquire rdlock first. */
> +void
> +xlate_actions_unsafe(struct xlate_in *xin, struct xlate_out *xout)
> +{
> + xlate_actions__(xin, xout);
> +}
> +
> /* Translates the 'ofpacts_len' bytes of "struct ofpacts" starting at
> 'ofpacts'
> * into datapath actions in 'odp_actions', using 'ctx'.
> *
> * The caller must take responsibility for eventually freeing 'xout', with
> * xlate_out_uninit(). */
> -void
> -xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
> +static void
> +xlate_actions__(struct xlate_in *xin, struct xlate_out *xout)
> + OVS_REQ_RDLOCK(xlate_rwlock)
> {
> struct flow_wildcards *wc = &xout->wc;
> struct flow *flow = &xin->flow;
> @@ -2537,8 +2557,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
>
> COVERAGE_INC(xlate_actions);
>
> - ovs_rwlock_rdlock(&xlate_rwlock);
> -
> /* Flow initialization rules:
> * - 'base_flow' must match the kernel's view of the packet at the
> * time that action processing starts. 'flow' represents any
> @@ -2690,7 +2708,60 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> memset(&wc->masks.regs, 0, sizeof wc->masks.regs);
>
> out:
> - ovs_rwlock_unlock(&xlate_rwlock);
> -
> rule_actions_unref(actions);
> }
> +
> +/* Sends 'packet' out 'ofport'.
> + * May modify 'packet'.
> + * Returns 0 if successful, otherwise a positive errno value. */
> +int
> +xlate_send_packet(const struct ofport_dpif *ofport, struct ofpbuf *packet)
> +{
> + uint64_t odp_actions_stub[1024 / 8];
> + struct xport *xport;
> + struct ofpbuf key, odp_actions;
> + struct dpif_flow_stats stats;
> + struct odputil_keybuf keybuf;
> + struct ofpact_output output;
> + struct xlate_out xout;
> + struct xlate_in xin;
> + struct flow flow;
> + union flow_in_port in_port_;
> + int error;
> +
> + ovs_rwlock_rdlock(&xlate_rwlock);
> + xport = xport_lookup(ofport);
> + if (!xport) {
> + error = EINVAL;
> + goto out;
> + }
> +
> + ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
> + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> +
> + /* Use OFPP_NONE as the in_port to avoid special packet processing. */
> + in_port_.ofp_port = OFPP_NONE;
> + flow_extract(packet, 0, 0, NULL, &in_port_, &flow);
> + odp_flow_key_from_flow(&key, &flow, ofp_port_to_odp_port(xport->xbridge,
> OFPP_LOCAL));
> + dpif_flow_stats_extract(&flow, packet, time_msec(), &stats);
> +
> + ofpact_init(&output.ofpact, OFPACT_OUTPUT, sizeof output);
> + output.port = xport->ofp_port;
> + output.max_len = 0;
> +
> + xlate_in_init(&xin, xport->xbridge->ofproto, &flow, NULL, 0, packet);
> + xin.ofpacts_len = sizeof output;
> + xin.ofpacts = &output.ofpact;
> + xin.resubmit_stats = &stats;
> + xlate_actions_unsafe(&xin, &xout);
> +
> + error = dpif_execute(xport->xbridge->dpif,
> + key.data, key.size,
> + xout.odp_actions.data, xout.odp_actions.size,
> + packet);
> + xlate_out_uninit(&xout);
> +
> +out:
> + ovs_rwlock_unlock(&xlate_rwlock);
> + return error;
> +}
> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> index a54a9e4..9e7a853 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -146,12 +146,15 @@ int xlate_receive(const struct dpif_backer *, struct
> ofpbuf *packet,
> struct ofproto_dpif **, odp_port_t *odp_in_port)
> OVS_EXCLUDED(xlate_rwlock);
>
> -void xlate_actions(struct xlate_in *, struct xlate_out *)
> +void xlate_actions_safe(struct xlate_in *, struct xlate_out *)
> OVS_EXCLUDED(xlate_rwlock);
> +void xlate_actions_unsafe(struct xlate_in *, struct xlate_out *)
> + OVS_REQ_RDLOCK(xlate_rwlock);
> void xlate_in_init(struct xlate_in *, struct ofproto_dpif *,
> const struct flow *, struct rule_dpif *,
> uint8_t tcp_flags, const struct ofpbuf *packet);
> void xlate_out_uninit(struct xlate_out *);
> void xlate_actions_for_side_effects(struct xlate_in *);
> void xlate_out_copy(struct xlate_out *dst, const struct xlate_out *src);
> +int xlate_send_packet(const struct ofport_dpif *, struct ofpbuf *);
> #endif /* ofproto-dpif-xlate.h */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 80874b8..42f3d98 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -540,9 +540,6 @@ static int expire(struct dpif_backer *);
> /* NetFlow. */
> static void send_netflow_active_timeouts(struct ofproto_dpif *);
>
> -/* Utilities. */
> -static int send_packet(const struct ofport_dpif *, struct ofpbuf *packet);
> -
> /* Global variables. */
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>
> @@ -2018,7 +2015,7 @@ send_bpdu_cb(struct ofpbuf *pkt, int port_num, void
> *ofproto_)
> VLOG_WARN_RL(&rl, "%s: cannot send BPDU on port %d "
> "with unknown MAC", ofproto->up.name, port_num);
> } else {
> - send_packet(ofport, pkt);
> + ofproto_dpif_send_packet(ofport, pkt);
> }
> }
> ofpbuf_delete(pkt);
> @@ -2614,7 +2611,7 @@ send_pdu_cb(void *port_, const void *pdu, size_t
> pdu_size)
> pdu_size);
> memcpy(packet_pdu, pdu, pdu_size);
>
> - send_packet(port, &packet);
> + ofproto_dpif_send_packet(port, &packet);
> ofpbuf_uninit(&packet);
> } else {
> VLOG_ERR_RL(&rl, "port %s: cannot obtain Ethernet address of iface "
> @@ -2651,7 +2648,7 @@ bundle_send_learning_packets(struct ofbundle *bundle)
> LIST_FOR_EACH (learning_packet, list_node, &packets) {
> int ret;
>
> - ret = send_packet(learning_packet->private_p, learning_packet);
> + ret = ofproto_dpif_send_packet(learning_packet->private_p,
> learning_packet);
> if (ret) {
> error = ret;
> n_errors++;
> @@ -2873,7 +2870,7 @@ port_run_fast(struct ofport_dpif *ofport)
>
> ofpbuf_init(&packet, 0);
> cfm_compose_ccm(ofport->cfm, &packet, ofport->up.pp.hw_addr);
> - send_packet(ofport, &packet);
> + ofproto_dpif_send_packet(ofport, &packet);
> ofpbuf_uninit(&packet);
> }
>
> @@ -2882,7 +2879,7 @@ port_run_fast(struct ofport_dpif *ofport)
>
> ofpbuf_init(&packet, 0);
> bfd_put_packet(ofport->bfd, &packet, ofport->up.pp.hw_addr);
> - send_packet(ofport, &packet);
> + ofproto_dpif_send_packet(ofport, &packet);
> ofpbuf_uninit(&packet);
> }
> }
> @@ -4200,7 +4197,7 @@ facet_check_consistency(struct facet *facet)
> /* Check the datapath actions for consistency. */
> rule_dpif_lookup(facet->ofproto, &facet->flow, NULL, &rule);
> xlate_in_init(&xin, facet->ofproto, &facet->flow, rule, 0, NULL);
> - xlate_actions(&xin, &xout);
> + xlate_actions_safe(&xin, &xout);
> rule_dpif_unref(rule);
>
> ok = ofpbuf_equal(&facet->xout.odp_actions, &xout.odp_actions)
> @@ -4284,7 +4281,7 @@ facet_revalidate(struct facet *facet)
> * emit a NetFlow expiration and, if so, we need to have the old state
> * around to properly compose it. */
> xlate_in_init(&xin, ofproto, &facet->flow, new_rule, 0, NULL);
> - xlate_actions(&xin, &xout);
> + xlate_actions_safe(&xin, &xout);
> flow_wildcards_or(&xout.wc, &xout.wc, &wc);
>
> /* A facet's slow path reason should only change under dramatic
> @@ -4909,7 +4906,7 @@ rule_dpif_execute(struct rule_dpif *rule, const struct
> flow *flow,
>
> xlate_in_init(&xin, ofproto, flow, rule, stats.tcp_flags, packet);
> xin.resubmit_stats = &stats;
> - xlate_actions(&xin, &xout);
> + xlate_actions_safe(&xin, &xout);
>
> execute_odp_actions(ofproto, flow, xout.odp_actions.data,
> xout.odp_actions.size, packet);
> @@ -4945,55 +4942,19 @@ rule_modify_actions(struct rule *rule_, bool
> reset_counters)
> /* Sends 'packet' out 'ofport'.
> * May modify 'packet'.
> * Returns 0 if successful, otherwise a positive errno value. */
> -static int
> -send_packet(const struct ofport_dpif *ofport, struct ofpbuf *packet)
> +int
> +ofproto_dpif_send_packet(const struct ofport_dpif *ofport, struct ofpbuf
> *packet)
> {
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
> - uint64_t odp_actions_stub[1024 / 8];
> - struct ofpbuf key, odp_actions;
> - struct dpif_flow_stats stats;
> - struct odputil_keybuf keybuf;
> - struct ofpact_output output;
> - struct xlate_out xout;
> - struct xlate_in xin;
> - struct flow flow;
> - union flow_in_port in_port_;
> int error;
>
> - ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
> - ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> -
> - /* Use OFPP_NONE as the in_port to avoid special packet processing. */
> - in_port_.ofp_port = OFPP_NONE;
> - flow_extract(packet, 0, 0, NULL, &in_port_, &flow);
> - odp_flow_key_from_flow(&key, &flow, ofp_port_to_odp_port(ofproto,
> - OFPP_LOCAL));
> - dpif_flow_stats_extract(&flow, packet, time_msec(), &stats);
> -
> - ofpact_init(&output.ofpact, OFPACT_OUTPUT, sizeof output);
> - output.port = ofport->up.ofp_port;
> - output.max_len = 0;
> -
> - xlate_in_init(&xin, ofproto, &flow, NULL, 0, packet);
> - xin.ofpacts_len = sizeof output;
> - xin.ofpacts = &output.ofpact;
> - xin.resubmit_stats = &stats;
> - xlate_actions(&xin, &xout);
> -
> - error = dpif_execute(ofproto->backer->dpif,
> - key.data, key.size,
> - xout.odp_actions.data, xout.odp_actions.size,
> - packet);
> - xlate_out_uninit(&xout);
> + error = xlate_send_packet(ofport, packet);
>
> - if (error) {
> - VLOG_WARN_RL(&rl, "%s: failed to send packet on port %s (%s)",
> - ofproto->up.name, netdev_get_name(ofport->up.netdev),
> - ovs_strerror(error));
> + if (!error) {
> + ofproto->stats.tx_packets++;
> + ofproto->stats.tx_bytes += packet->size;
> }
>
> - ofproto->stats.tx_packets++;
> - ofproto->stats.tx_bytes += packet->size;
> return error;
> }
>
> @@ -5075,7 +5036,7 @@ packet_out(struct ofproto *ofproto_, struct ofpbuf
> *packet,
> xin.ofpacts_len = ofpacts_len;
> xin.ofpacts = ofpacts;
>
> - xlate_actions(&xin, &xout);
> + xlate_actions_safe(&xin, &xout);
> dpif_execute(ofproto->backer->dpif, key.data, key.size,
> xout.odp_actions.data, xout.odp_actions.size, packet);
> xlate_out_uninit(&xout);
> @@ -5495,7 +5456,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const
> struct flow *flow,
> trace.xin.resubmit_hook = trace_resubmit;
> trace.xin.report_hook = trace_report;
>
> - xlate_actions(&trace.xin, &trace.xout);
> + xlate_actions_safe(&trace.xin, &trace.xout);
> flow_wildcards_or(&trace.xout.wc, &trace.xout.wc, &wc);
>
> ds_put_char(ds, '\n');
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index 14a9669..0863efd 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -95,6 +95,7 @@ bool vsp_adjust_flow(const struct ofproto_dpif *, struct
> flow *);
>
> void ofproto_dpif_send_packet_in(struct ofproto_dpif *,
> struct ofputil_packet_in *pin);
> +int ofproto_dpif_send_packet(const struct ofport_dpif *, struct ofpbuf *);
> void ofproto_dpif_flow_mod(struct ofproto_dpif *, struct ofputil_flow_mod *);
>
> struct ofport_dpif *odp_port_to_ofport(const struct dpif_backer *,
> odp_port_t);
> --
> 1.7.9.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
