Hi Daniel,
Thank you for taking a look!
On 2018/08/07 23:26, Daniel Borkmann wrote:
> On 08/03/2018 09:58 AM, Toshiaki Makita wrote:
> [...]
>> +
>> +static struct sk_buff *veth_xdp_rcv_skb(struct veth_priv *priv,
>> + struct sk_buff *skb)
>> +{
>> + u32 pktlen, headroom, act, metalen;
>> + void *orig_data, *orig_data_end;
>> + struct bpf_prog *xdp_prog;
>> + int mac_len, delta, off;
>> + struct xdp_buff xdp;
>> +
>> + rcu_read_lock();
>> + xdp_prog = rcu_dereference(priv->xdp_prog);
>> + if (unlikely(!xdp_prog)) {
>> + rcu_read_unlock();
>> + goto out;
>> + }
>> +
>> + mac_len = skb->data - skb_mac_header(skb);
>> + pktlen = skb->len + mac_len;
>> + headroom = skb_headroom(skb) - mac_len;
>> +
>> + if (skb_shared(skb) || skb_head_is_locked(skb) ||
>> + skb_is_nonlinear(skb) || headroom < XDP_PACKET_HEADROOM) {
>
> Hmm, I think this is not fully correct. What happens if you have cloned skbs
> as
> e.g. the case with TCP? This would also need a full expensive unclone to make
> the
> data private as expected by XDP (this is basically a similar issue in generic
> XDP).
Well, cloned is checked in skb_head_is_locked() so TCP packets are
always uncloned here.
> It may potentially be worth to also share the code here with generic XDP
> implementation given it's quite similar?
For now I'm not sharing the code because of two reasons.
One is that as you say generic XDP skips cloned packets. I traced the
reason and it seems it is to skip packets redirected by act_mirred.
https://patchwork.ozlabs.org/patch/750127/
The assumption that no one provides cloned skbs other than mirred breaks
when generic XDP added support for virtual devices, but it is still
valid that we should skip packets redirected by mirred if we want to be
in line with driver XDP. So I'm thinking generic XDP needs something
more than just uncloning cloned skbs.
The other reason is performance for REDIRECT. We can make use of bulk
redirection in driver XDP, but it requires xdp_frames which requires
non-kmallocked skb head. This is different from generic XDP which allows
kmallocked skb head and uses kmalloc if head reallocation is needed.
--
Toshiaki Makita
