[ovs-dev] [PATCHv2 3/3] tunnel: set skb mark for IPsec tunnel packets

Jesse Gross jesse at nicira.com
Fri Feb 15 05:07:37 UTC 2013


On Thu, Feb 14, 2013 at 6:20 PM, Ansis Atteka <aatteka at nicira.com> wrote:
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 7e48981..08195b7 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> +void
> +odp_put_skb_mark_action(const uint32_t skb_mark,
> +                        struct ofpbuf *odp_actions)
> +{
> +    size_t offset = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_SET);
> +    nl_msg_put_u32(odp_actions, OVS_KEY_ATTR_SKB_MARK, skb_mark);
> +    nl_msg_end_nested(odp_actions, offset);
> +}

I think it would be good to reuse more of our existing functions here
- i.e. commit_set_skb_mark_action() calls odp_put_skb_mark_action()
which in turn calls commit_set_action().

> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index b1ec3fb..727ddef 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5512,8 +5512,9 @@ send_packet(const struct ofport_dpif *ofport, struct ofpbuf *packet)
>
>      if (ofport->tnl_port) {
>          struct dpif_flow_stats stats;
> +        uint32_t skb_mark;
>
> -        odp_port = tnl_port_send(ofport->tnl_port, &flow);
> +        odp_port = tnl_port_send(ofport->tnl_port, &flow, &skb_mark);
>          if (odp_port == OVSP_NONE) {
>              return ENODEV;
>          }
> @@ -5521,6 +5522,7 @@ send_packet(const struct ofport_dpif *ofport, struct ofpbuf *packet)
>          dpif_flow_stats_extract(&flow, packet, time_msec(), &stats);
>          netdev_vport_inc_tx(ofport->up.netdev, &stats);
>          odp_put_tunnel_action(&flow.tunnel, &odp_actions);
> +        odp_put_skb_mark_action(skb_mark, &odp_actions);

Couldn't we have tnl_port_send() write skb_mark directly into the flow
like is done with the tunnel parameters?  That would be a little more
consistent and reduce the number of arguments that we have to pass
around.

> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index 5a4607e..ee73525 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -47,6 +46,7 @@ struct tnl_match {
>      ovs_be32 ip_dst;
>      uint32_t odp_port;
>      bool in_key_flow;
> +    uint32_t skb_mark;

It would be good to keep these in sorted order by size (i.e. after
odp_port) to reduce the possibility of holes in the struct in the
future.

> diff --git a/ofproto/tunnel.h b/ofproto/tunnel.h
> index acb69a8..38c7bec 100644
> --- a/ofproto/tunnel.h
> +++ b/ofproto/tunnel.h
> @@ -25,6 +25,9 @@
>   * These functions emulate tunnel virtual ports based on the outer
>   * header information from the kernel. */
>
> +/* skb mark used for IPsec tunnel packets */
> +#define IPSEC_MARK 1

I think we probably could just keep this in tunnel.c since nothing else uses it.



More information about the dev mailing list