Hi, On Mon, 2018-10-22 at 13:04 -0600, Subash Abhinov Kasiviswanathan wrote: > On 2018-10-19 08:25, Paolo Abeni wrote: > > In some scenarios, the GRO engine can assemble an UDP GRO packet > > that ultimately lands on a non GRO-enabled socket. > > This patch tries to address the issue explicitly checking for the UDP > > socket features before enqueuing the packet, and eventually segmenting > > the unexpected GRO packet, as needed. > > > > We must also cope with re-insertion requests: after segmentation the > > UDP code calls the helper introduced by the previous patches, as > > needed. > > > > Signed-off-by: Paolo Abeni <pab...@redhat.com> > > --- > > +static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff > > *skb) > > +{ > > + return !udp_sk(sk)->gro_enabled && skb_is_gso(skb) && > > + skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4; > > +} > > + > > +static inline struct sk_buff *udp_rcv_segment(struct sock *sk, > > + struct sk_buff *skb) > > +{ > > + struct sk_buff *segs; > > + > > + /* the GSO CB lays after the UDP one, no need to save and restore > > any > > + * CB fragment, just initialize it > > + */ > > + segs = __skb_gso_segment(skb, NETIF_F_SG, false); > > + if (unlikely(IS_ERR(segs))) > > + kfree_skb(skb); > > + else if (segs) > > + consume_skb(skb); > > + return segs; > > +} > > + > > + > > Hi Paolo > > Do we need to check for IS_ERR_OR_NULL(segs)
Yes, thanks. (also Williem already noted the above) > > > > +void ip_protocol_deliver_rcu(struct net *net, struct sk_buff *skb, int > > proto); > > + > > +static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > > +{ > > + struct sk_buff *next, *segs; > > + int ret; > > + > > + if (likely(!udp_unexpected_gso(sk, skb))) > > + return udp_queue_rcv_one_skb(sk, skb); > > +static int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > > +{ > > + struct sk_buff *next, *segs; > > + int ret; > > + > > + if (likely(!udp_unexpected_gso(sk, skb))) > > + return udpv6_queue_rcv_one_skb(sk, skb); > > + > > Is the "likely" required here? Not required, but currently helpful IMHO, as we should hit the above only on unlikey and really unwonted configuration. Note that only SKB_GSO_UDP_L4 GSO packets will not match the above likely condition. > HW can coalesce all incoming streams of UDP and may not know the socket > state. > In that case, a socket not having UDP GRO option might see a penalty > here. Really? Is there any HW creating SKB_GSO_UDP_L4 packets on RX? if the HW is doing that, without this patch, I think it's breaking existing applications (which may expext that the read UDP frame length implicitly describe the application level message length). Cheers, Paolo