| Message ID | 1395263249-16450-1-git-send-email-zoltan.kiss@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/19/2014 10:07 PM, Zoltan Kiss wrote: > skb_zerocopy can copy elements of the frags array between skbs, but it doesn't > orphan them. Also, it doesn't handle errors, so this patch takes care of that > as well. > > Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> Acked-by: Thomas Graf <tgraf@redhat.com> > --- > + if (unlikely(skb_orphan_frags(to, GFP_ATOMIC))) { > + skb_tx_error(to); > + return -ENOMEM; > + } Did you consider calling skb_tx_error() for Netlink message allocation failures for the upcall as well? That memory pressure is currently not reported back. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/20/2014 01:16 PM, Thomas Graf wrote: > On 03/19/2014 10:07 PM, Zoltan Kiss wrote: >> skb_zerocopy can copy elements of the frags array between skbs, but it >> doesn't >> orphan them. Also, it doesn't handle errors, so this patch takes care >> of that >> as well. >> >> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> > > Acked-by: Thomas Graf <tgraf@redhat.com> I take this back ;) >> --- >> + if (unlikely(skb_orphan_frags(to, GFP_ATOMIC))) { >> + skb_tx_error(to); >> + return -ENOMEM; >> + } Just noticed that you orphan the Netlink skb frags which do not exist yet instead of the source skb frags. > Did you consider calling skb_tx_error() for Netlink message > allocation failures for the upcall as well? That memory pressure > is currently not reported back. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/03/14 12:33, Thomas Graf wrote: > On 03/20/2014 01:16 PM, Thomas Graf wrote: >> On 03/19/2014 10:07 PM, Zoltan Kiss wrote: >>> skb_zerocopy can copy elements of the frags array between skbs, but it >>> doesn't >>> orphan them. Also, it doesn't handle errors, so this patch takes care >>> of that >>> as well. >>> >>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> >> >> Acked-by: Thomas Graf <tgraf@redhat.com> > > I take this back ;) > >>> --- >>> + if (unlikely(skb_orphan_frags(to, GFP_ATOMIC))) { >>> + skb_tx_error(to); >>> + return -ENOMEM; >>> + } > > Just noticed that you orphan the Netlink skb frags which do not > exist yet instead of the source skb frags. Oh, sorry, I'll fix this up. > >> Did you consider calling skb_tx_error() for Netlink message >> allocation failures for the upcall as well? That memory pressure >> is currently not reported back. Yeah, that makes sense, I'll fix it the callers. Btw. I guess that just provides some statistical data for the creator of the skb. At least for netback it only increment a stat counter. Zoli -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 03db95a..35c4e85 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -2508,8 +2508,8 @@ int skb_splice_bits(struct sk_buff *skb, unsigned int offset, unsigned int flags); void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to); unsigned int skb_zerocopy_headlen(const struct sk_buff *from); -void skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, - int len, int hlen); +int skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, + int len, int hlen); void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len); int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen); void skb_scrub_packet(struct sk_buff *skb, bool xnet); diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 3f14c63..5cc99b1 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -2127,25 +2127,31 @@ EXPORT_SYMBOL_GPL(skb_zerocopy_headlen); * * The `hlen` as calculated by skb_zerocopy_headlen() specifies the * headroom in the `to` buffer. + * + * Return value: + * 0: everything is OK + * -ENOMEM: couldn't orphan frags of from due to lack of memory + * -EFAULT: skb_copy_bits() found some problem with geometry */ -void +int skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen) { int i, j = 0; int plen = 0; /* length of skb->head fragment */ + int ret; struct page *page; unsigned int offset; BUG_ON(!from->head_frag && !hlen); /* dont bother with small payloads */ - if (len <= skb_tailroom(to)) { - skb_copy_bits(from, 0, skb_put(to, len), len); - return; - } + if (len <= skb_tailroom(to)) + return skb_copy_bits(from, 0, skb_put(to, len), len); if (hlen) { - skb_copy_bits(from, 0, skb_put(to, hlen), hlen); + ret = skb_copy_bits(from, 0, skb_put(to, hlen), hlen); + if (unlikely(ret)) + return ret; len -= hlen; } else { plen = min_t(int, skb_headlen(from), len); @@ -2163,6 +2169,11 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen) to->len += len + plen; to->data_len += len + plen; + if (unlikely(skb_orphan_frags(to, GFP_ATOMIC))) { + skb_tx_error(to); + return -ENOMEM; + } + for (i = 0; i < skb_shinfo(from)->nr_frags; i++) { if (!len) break; @@ -2173,6 +2184,8 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen) j++; } skb_shinfo(to)->nr_frags = j; + + return 0; } EXPORT_SYMBOL_GPL(skb_zerocopy); diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c index f072fe8..ea02d16 100644 --- a/net/netfilter/nfnetlink_queue_core.c +++ b/net/netfilter/nfnetlink_queue_core.c @@ -488,7 +488,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, nla->nla_type = NFQA_PAYLOAD; nla->nla_len = nla_attr_size(data_len); - skb_zerocopy(skb, entskb, data_len, hlen); + if (skb_zerocopy(skb, entskb, data_len, hlen)) + goto nla_put_failure; } nlh->nlmsg_len = skb->len; diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index c53fe0c..9a9f668 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -464,7 +464,9 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, } nla->nla_len = nla_attr_size(skb->len); - skb_zerocopy(user_skb, skb, skb->len, hlen); + err = skb_zerocopy(user_skb, skb, skb->len, hlen); + if (err) + goto out; /* Pad OVS_PACKET_ATTR_PACKET if linear copy was performed */ if (!(dp->user_features & OVS_DP_F_UNALIGNED)) {
skb_zerocopy can copy elements of the frags array between skbs, but it doesn't orphan them. Also, it doesn't handle errors, so this patch takes care of that as well. Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> --- v2: orphan the frags right before touching the frags -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html