On 12/03/13 at 09:43pm, Jesse Gross wrote: Thanks for merging a first set of patches
> On Sat, Nov 30, 2013 at 4:21 AM, Thomas Graf <tg...@suug.ch> wrote: > > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c > > index 8eaa39a..867edf1 100644 > > --- a/net/openvswitch/datapath.c > > +++ b/net/openvswitch/datapath.c > > +static int queue_gso_packets(struct datapath *, struct net *, int > > dp_ifindex, > > + struct sk_buff *, const struct dp_upcall_info > > *); > > +static int queue_userspace_packet(struct datapath *, struct net *, > > + int dp_ifindex, struct sk_buff *, > > const struct dp_upcall_info *); > > Should we drop the dp_ifindex arguments from these functions? It > should be trivially derivable from struct datapath. Sounds good > > > static size_t upcall_msg_size(const struct sk_buff *skb, > > - const struct nlattr *userdata) > > + const struct nlattr *userdata, > > + unsigned int hdrlen) > > I think that 'skb' is now unused. Right > > > @@ -427,7 +429,21 @@ static int queue_userspace_packet(struct net *net, int > > dp_ifindex, > > goto out; > > } > > > > - len = upcall_msg_size(skb, upcall_info->userdata); > > + /* Complete checksum if needed */ > > + if (skb->ip_summed == CHECKSUM_PARTIAL && > > + (err = skb_checksum_help(skb))) > > + goto out; > > I think that we can remove the hardware features argument to > __skb_gso_segment() in queue_gso_packet(). It was there to take > advantage of the copy and checksum optimization but that's no longer > present. In the case of a mapped skb we could still make use of it as we copy the packet into the headroom. It would mean integrating the logic into skb_zerocopy() though so I'd say we stay with what you propose. It shouldn't be the fast path anyway. > > @@ -447,13 +463,17 @@ static int queue_userspace_packet(struct net *net, > > int dp_ifindex, > > nla_len(upcall_info->userdata), > > nla_data(upcall_info->userdata)); > > > > - nla = __nla_reserve(user_skb, OVS_PACKET_ATTR_PACKET, skb->len); > > + /* Only reserve room for attribute header, packet data is added > > + * in skb_zerocopy() */ > > + if (!(nla = nla_reserve(user_skb, OVS_PACKET_ATTR_PACKET, 0))) > > + goto out; > > Does this initialized 'err' on failure? Good catch, I'll fix this up as well. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
