On Tue, Jun 7, 2016 at 10:53 PM, William Tu <u9012...@gmail.com> wrote:
> The patch adds a new action to support packet truncation. The new action
> is formatted as 'output(port=n,max_len=m)', as output to port n, with
> packet size being MIN(original_size, m).
>
> One use case is to enable port mirroring to send smaller packets to the
> destination port so that only useful packet information is mirrored/copied,
> saving some performance overhead of copying entire packet payload. Example
> use case is below as well as shown in the testcases:
>
> - Output to port 1 with max_len 100 bytes.
> - The output packet size on port 1 will be MIN(original_packet_size, 100).
> # ovs-ofctl add-flow br0 'actions=output(port=1,max_len=100)'
>
> - The scope of max_len is limited to output action itself. The following
> packet size of output:1 and output:2 will be intact.
> # ovs-ofctl add-flow br0 \
> 'actions=output(port=1,max_len=100),output:1,output:2'
> - The Datapath actions shows:
> # Datapath actions: trunc(100),1,1,2
>
> Signed-off-by: William Tu <u9012...@gmail.com>
> ---
Can you send next version against net-next tree? All feature patches
needs to be pushed upstream OVS first.
...
> diff --git a/datapath/actions.c b/datapath/actions.c
> index dcf8591..92ee3f9 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -778,6 +778,7 @@ static int output_userspace(struct datapath *dp, struct
> sk_buff *skb,
> memset(&upcall, 0, sizeof(upcall));
> upcall.cmd = OVS_PACKET_CMD_ACTION;
> upcall.mru = OVS_CB(skb)->mru;
> + upcall.cutlen = OVS_CB(skb)->cutlen;
>
> for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
> a = nla_next(a, &rem)) {
> @@ -854,10 +855,17 @@ static int sample(struct datapath *dp, struct sk_buff
> *skb,
> return 0;
>
> /* The only known usage of sample action is having a single user-space
> + * action, or having a truncate action followed by a single user-space
> * action. Treat this usage as a special case.
> * The output_userspace() should clone the skb to be sent to the
> - * user space. This skb will be consumed by its caller.
> - */
> + * user space. This skb will be consumed by its caller. */
> + if (unlikely(nla_type(a) == OVS_ACTION_ATTR_TRUNC)) {
> + struct ovs_action_trunc *trunc = nla_data(a);
> + OVS_CB(skb)->cutlen = skb->len > trunc->max_len ?
> + skb->len - trunc->max_len : 0;
> + a = nla_next(a, &rem);
> + }
> +
> if (likely(nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
> nla_is_last(a, rem)))
> return output_userspace(dp, skb, key, a, actions,
> actions_len);
> @@ -1040,10 +1048,15 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> for (a = attr, rem = len; rem > 0;
> a = nla_next(a, &rem)) {
> int err = 0;
> + int cutlen = OVS_CB(skb)->cutlen;
>
> if (unlikely(prev_port != -1)) {
> struct sk_buff *out_skb = skb_clone(skb, GFP_ATOMIC);
>
> + if (cutlen > 0) {
> + pskb_trim(out_skb, out_skb->len - cutlen);
> + OVS_CB(skb)->cutlen = 0;
> + }
> if (out_skb)
> do_output(dp, out_skb, prev_port, key);
>
Lets move the pskb_trim() call inside do_output(). So that NULL skb
is not passed to the function.
> @@ -1055,6 +1068,16 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> prev_port = nla_get_u32(a);
> break;
>
> + case OVS_ACTION_ATTR_TRUNC: {
> + struct ovs_action_trunc *trunc = nla_data(a);
> +
> + if (trunc->max_len < ETH_MIN_FRAME_LEN)
> + return -EINVAL;
Does max_len still needs to be check after checking the at flow install.
> + OVS_CB(skb)->cutlen = skb->len > trunc->max_len ?
> + skb->len - trunc->max_len : 0;
> + break;
> + }
> +
> case OVS_ACTION_ATTR_USERSPACE:
> output_userspace(dp, skb, key, a, attr, len);
> break;
> @@ -1125,8 +1148,15 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> }
> }
>
> - if (prev_port != -1)
> + if (prev_port != -1) {
> + uint32_t cutlen = OVS_CB(skb)->cutlen;
> +
> + if (cutlen > 0) {
> + pskb_trim(skb, skb->len - cutlen);
> + OVS_CB(skb)->cutlen = 0;
> + }
> do_output(dp, skb, prev_port, key);
> + }
By moving pskb_trim() to do_output() we can avoid the duplicate code.
> else
> consume_skb(skb);
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 5bec072..958dfb8 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -280,6 +280,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct
> sw_flow_key *key)
> upcall.cmd = OVS_PACKET_CMD_MISS;
> upcall.portid = ovs_vport_find_upcall_portid(p, skb);
> upcall.mru = OVS_CB(skb)->mru;
> + upcall.cutlen = OVS_CB(skb)->cutlen;
> error = ovs_dp_upcall(dp, skb, key, &upcall);
> if (unlikely(error))
> kfree_skb(skb);
> @@ -409,6 +410,10 @@ static size_t upcall_msg_size(const struct
> dp_upcall_info *upcall_info,
> if (upcall_info->mru)
> size += nla_total_size(sizeof(upcall_info->mru));
>
> + /* OVS_PACKET_ATTR_CUTLEN */
> + if (upcall_info->cutlen)
> + size += nla_total_size(sizeof(upcall_info->cutlen));
> +
I think this should be part of separate patch. This is not required
for truncate support.
> return size;
> }
>
> @@ -439,6 +444,7 @@ static int queue_userspace_packet(struct datapath *dp,
> struct sk_buff *skb,
> size_t len;
> unsigned int hlen;
> int err, dp_ifindex;
> + int cutlen = OVS_CB(skb)->cutlen;
>
> dp_ifindex = get_dpifindex(dp);
> if (!dp_ifindex)
> @@ -475,6 +481,9 @@ static int queue_userspace_packet(struct datapath *dp,
> struct sk_buff *skb,
> else
> hlen = skb->len;
>
> + if (cutlen > 0)
> + hlen -= cutlen;
> +
> len = upcall_msg_size(upcall_info, hlen);
> user_skb = genlmsg_new_unicast(len, &info, GFP_ATOMIC);
> if (!user_skb) {
> @@ -525,6 +534,16 @@ static int queue_userspace_packet(struct datapath *dp,
> struct sk_buff *skb,
> pad_packet(dp, user_skb);
> }
>
> + /* Add OVS_PACKET_ATTR_CUTLEN */
> + if (upcall_info->cutlen) {
> + if (nla_put_u16(user_skb, OVS_PACKET_ATTR_CUTLEN,
> + upcall_info->cutlen)) {
> + err = -ENOBUFS;
> + goto out;
> + }
> + pad_packet(dp, user_skb);
> + }
> +
> /* Only reserve room for attribute header, packet data is added
> * in skb_zerocopy()
> */
> @@ -532,9 +551,10 @@ static int queue_userspace_packet(struct datapath *dp,
> struct sk_buff *skb,
> err = -ENOBUFS;
> goto out;
> }
> - nla->nla_len = nla_attr_size(skb->len);
> + nla->nla_len = nla_attr_size(skb->len - cutlen);
>
> - err = skb_zerocopy(user_skb, skb, skb->len, hlen);
> + err = skb_zerocopy(user_skb, skb,
> + (cutlen > 0) ? hlen : skb->len, hlen);
I do not think we need to compare cut-len with zero, we can just
subtract it from skb->len.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
