lkml.org 
[lkml]   [2017]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH net-next V3] tun: add eBPF based queue selection method
From
Date


On 2017年12月05日 08:16, Willem de Bruijn wrote:
> On Mon, Dec 4, 2017 at 4:31 AM, Jason Wang <jasowang@redhat.com> wrote:
>> This patch introduces an eBPF based queue selection method. With this,
>> the policy could be offloaded to userspace completely through a new
>> ioctl TUNSETSTEERINGEBPF.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> +static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
>> +{
>> + struct tun_steering_prog *prog;
>> + u16 ret = 0;
>> +
>> + prog = rcu_dereference(tun->steering_prog);
>> + if (prog)
>> + ret = bpf_prog_run_clear_cb(prog->prog, skb);
> This dereferences tun->steering_prog for a second time. It is safe
> in this load balancing case to assign a few extra packets to queue 0.
> But the issue can also be avoided by replacing the function with a
> direct call in tun_net_xmit:
>
> struct tun_steering_prog *s = rcu_dereference(tun->steering_prog);
> if (s)
> ret = bpf_prog_run_clear_cb(s->prog, skb) % tun->numqueues;

Right.

>
>> /* Net device start xmit */
>> -static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>> +static void tun_automq_xmit(struct tun_struct *tun, struct sk_buff *skb)
>> {
>> - struct tun_struct *tun = netdev_priv(dev);
>> - int txq = skb->queue_mapping;
>> - struct tun_file *tfile;
>> - u32 numqueues = 0;
>> -
>> - rcu_read_lock();
>> - tfile = rcu_dereference(tun->tfiles[txq]);
>> - numqueues = READ_ONCE(tun->numqueues);
>> -
>> - /* Drop packet if interface is not attached */
>> - if (txq >= numqueues)
>> - goto drop;
>> -
>> #ifdef CONFIG_RPS
>> - if (numqueues == 1 && static_key_false(&rps_needed)) {
>> + if (tun->numqueues == 1 && static_key_false(&rps_needed)) {
>> /* Select queue was not called for the skbuff, so we extract the
>> * RPS hash and save it into the flow_table here.
>> */
>> @@ -969,6 +986,26 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>> }
>> }
>> #endif
>> +}
>> +
>> +/* Net device start xmit */
>> +static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> + struct tun_struct *tun = netdev_priv(dev);
>> + int txq = skb->queue_mapping;
>> + struct tun_file *tfile;
>> + u32 numqueues = 0;
>> +
>> + rcu_read_lock();
>> + tfile = rcu_dereference(tun->tfiles[txq]);
>> + numqueues = READ_ONCE(tun->numqueues);
> Now tun->numqueues is read twice, reversing commit fa35864e0bb7
> ("tuntap: Fix for a race in accessing numqueues"). I don't see anything
> left that would cause a divide by zero after the relevant code was
> converted from divide to multiple and subsequently even removed.
>
> But if it's safe to read multiple times, might as well remove the READ_ONCE.

Good point, but READ_ONCE() is not something new, we'd better change
this in another patch.

>
>> @@ -1551,7 +1588,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>> int copylen;
>> bool zerocopy = false;
>> int err;
>> - u32 rxhash;
>> + u32 rxhash = 0;
>> int skb_xdp = 1;
>> bool frags = tun_napi_frags_enabled(tun);
>>
>> @@ -1739,7 +1776,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>> rcu_read_unlock();
>> }
>>
>> - rxhash = __skb_get_hash_symmetric(skb);
>> + rcu_read_lock();
>> + if (!rcu_dereference(tun->steering_prog))
>> + rxhash = __skb_get_hash_symmetric(skb);
>> + rcu_read_unlock();
>>
>> if (frags) {
>> /* Exercise flow dissector code path. */
>> @@ -1783,7 +1823,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>> u64_stats_update_end(&stats->syncp);
>> put_cpu_ptr(stats);
>>
>> - tun_flow_update(tun, rxhash, tfile);
>> + if (rxhash)
>> + tun_flow_update(tun, rxhash, tfile);
>> +
> Nit: zero is a valid hash? In which case, an int64_t initialized to -1 is the
> safer check.

Looks not? E.g looking at __flow_hash_from_keys() it did:

static inline u32 __flow_hash_from_keys(struct flow_keys *keys, u32 keyval)
{
    u32 hash;

    __flow_hash_consistentify(keys);

    hash = __flow_hash_words(flow_keys_hash_start(keys),
                 flow_keys_hash_length(keys), keyval);
    if (!hash)
        hash = 1;

    return hash;
}

Thanks

\
 
 \ /
  Last update: 2017-12-05 08:30    [W:0.074 / U:12.240 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site