Re: [PATCH v5 1/2] batman-adv: split DAT cache into DAT cache and DAT DHT

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]

On Monday, 28 October 2024 14:25:36 CET 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()]


Looks like this was never answered and also not handled in the v6 version of the patch.

[...]
> > --- a/net/batman-adv/types.h
> > +++ b/net/batman-adv/types.h
> > @@ -1231,8 +1231,11 @@ struct batadv_priv_dat {
> >   	/** @addr: node DAT address */
> >   	batadv_dat_addr_t addr;
> >   
> > -	/** @hash: hashtable representing the local ARP cache */
> > -	struct batadv_hashtable *hash;
> > +	/** @cache_hash: hashtable representing the local ARP cache */
> > +	struct batadv_hashtable *cache_hash;
> > +
> > +	/** @dht_hash: hashtable representing the local DAT DHT */
> > +	struct batadv_hashtable *dht_hash;
> >   
> >   	/** @work: work queue callback item for cache purging */
> >   	struct delayed_work work;
> 
> 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?
> 
> 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?


Looks like this was never answered.


Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]