Sorry, had overlooked the inline comments in the code. Replies follow below.
On Mon, Oct 28, 2024 at 02:25:36PM +0100, Antonio Quartulli wrote:
[...]
+/**
- batadv_dat_get_softif() - get the soft interface from a netlink callback
- @cb: callback structure containing arguments
- Return: The soft interface on success or an error pointer otherwise.
- */
+static struct net_device *batadv_dat_get_softif(struct netlink_callback *cb) +{
- struct net *net = sock_net(cb->skb->sk);
- struct net_device *soft_iface;
- int ifindex;
- ifindex = batadv_netlink_get_ifindex(cb->nlh,
BATADV_ATTR_MESH_IFINDEX);- if (!ifindex)
return ERR_PTR(-EINVAL);- soft_iface = dev_get_by_index(net, ifindex);
- if (!soft_iface)
return ERR_PTR(-ENODEV);- if (!batadv_softif_is_valid(soft_iface)) {
dev_put(soft_iface);return ERR_PTR(-ENODEV);- }
- return soft_iface;
+}
I don't think this function is DAT specific at all. Moreover, the very same code (which I think you are re-using here) appears in batadv_netlink_dump_hardif().
I think it'd make more sense to factor it out and create a helper out of it (place it in netlink.c?). This way we avoid code duplication.
[I might be wrong but 90% of the work already is in batadv_get_softif_from_info()]
Good idea, will do in v7! There are actually many more places that have basically the same code... :D.
I can see that most of the code in this patch is about handling two tables (in a generic fashion) instead of one.
One functional change I am seeing is also that before this patch batman-adv would store/cache any ARP information coming from the mesh. While now this happens only for the DHT PUT. Am I right?
Partially. For the DAT Cache part, it should be the same. In the batadv_dat_snoop_incoming_* functions we still add to the DAT cache table like we did before, unless I'm missing something.
For the new DAT DHT part, correct, there are the following cases where we don't update this anymore (immediately):
* incoming ARP request: we don't add/update the ARP source to DAT DHT * incoming ARP reply: we only add/update the ARP source+destination to DAT DHT if it was via a DHT PUT * incoming DHCP ACK: we don't add/update to DAT DHT
However these packets must have come from somewhere. So their originator should have sent a DHT PUT, too? If they arrive a bit later, shouldn't be an issue as these packets at least updated the DHT Cache.
If the according DHT PUT got lost on the way and we didn't have these entries in our DHT yet, shouldn't be an issue as we have two more DAT DHT candidates.
The only case where I could see it making a (small?) difference in practice is if a DHT PUT got lost and it would have changed the MAC address for the DAT DHT entry. But I'm not sure if that would justify extra lines of code right now? (For that use-case we should probably also have a majority vote of DAT_CACHE_REPLY from DAT candidates, instead of updating our DAT cache with the first reply we get - but we didn't do that so far either, for simplicity's sake.)
If that's the case, it means we may issue more DHT_GETs (and possibly ARP requests) because we lost a chance to cache a bit more that what the DHT stores. Does it make sense?
I don't think we would issue more DHT_GETs, as we at least update the DAT cache table like we did before. And only if we don't have an entry in the DAT Cache then we would trigger DHT_GETs?
Hope that makes sense?
Regards, Linus