On 16/09/14 11:17, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 11:04:13AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On the lowpan_give_skb_to_devices change.
>>
>> As we are iterating over a list of lowpan_devices and could potentially copy the skb more than once, what happens if the first device returns NET_RX_DROP and then the second time it return NET_RX_SUCCESS? The stat variable is overwritten so stat only ever reflects the return value of netif_rx for the last device?
>>
>> Maybe it's better to completely remove the if else at the end and always consume the skb? For the case whereskb_copy fails then we should kfree_skb,
>> e.g.
>>
>> static int lowpan_give_skb_to_devices(struct sk_buff *skb,
>> struct net_device *dev)
>> {
>> struct lowpan_dev_record *entry;
>> struct sk_buff *skb_cp;
>> int stat = NET_RX_SUCCESS;
>>
>> rcu_read_lock();
>> list_for_each_entry_rcu(entry, &lowpan_devices, list)
>> if (lowpan_dev_info(entry->ldev)->real_dev == skb->dev) {
>> skb_cp = skb_copy(skb, GFP_ATOMIC);
>> if (!skb_cp) {
>> kfree_skb(skb);
>> rcu_read_unlock();
>> return NET_RX_DROP;
>> }
>>
>> skb_cp->dev = entry->ldev;
>> stat = netif_rx(skb_cp);
> here we should do a:
>
> if (stat == NET_RX_DROP)
> kfree_skb(skb_cp);
>
> or? It doesn't deliver and then we "could" lost the pointer.
Doesn't netif_rx always free the buffer?
>> }
>> rcu_read_unlock();
>>
>> consume_skb(skb);
>>
>> return stat;
>> }
>>
>>
>> what are your thoughts?
>>
> for consume_skb:
>
> for me it's ok to make this behaviour. We never deliver the skb, always
> skb_cp. So if we are before the deliver call (netif_rx) this should
> never failed and we should consume the skb from which we did some copies.
yep
>
>
>
>
> btw.
>
> I see now that's skb_copy... mhhh. But this another issue. There exist
> skb_clone and skb_copy. skb_clone make a copy of struct sk_buff and data
> buffer is shared. I am currently not sure if we also can use a skb_clone
> here instead skb_copy, because the IPv6 doesn't manipulate the data buffer
> (I think it doesn't change the data buffer -> only parse) I need to think
> more about this, just a performance hint. But I really also doesn't know
> what sense makes multiple lowpan devices for one wpan interface. :-)
skb_clone could be a future patch.
Also I have been wondering why there are multiple lowpan device multiplexed onto a wpan. Again maybe a future patch.
>
> - Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
- Martin.
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
