[PATCH 9/9] Add cache refreshing logic

Jukka Rissanen jukka.rissanen at linux.intel.com
Mon Jan 9 03:47:04 PST 2012


Hi Arjan,


On 01/07/2012 03:07 AM, Arjan van de Ven wrote:
> This patch adds logic to the DNS cache where it will refresh, rather than purge,
> popular DNS entries when they expire. This will cut down latency for the user,
> since he doesn't have to wait for a DNS resolve for his popular entries.
>
> It also makes changing DNS servers (say, when joining a VPN) less expensive,
> since the popular entries from the cache will be refreshed after the join,
> rather than just being dropped
> ---
>   src/dnsproxy.c |  140 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>   1 files changed, 131 insertions(+), 9 deletions(-)
>
> diff --git a/src/dnsproxy.c b/src/dnsproxy.c
> index 59e5582..ac36bf0 100644
> --- a/src/dnsproxy.c
> +++ b/src/dnsproxy.c
> @@ -33,6 +33,7 @@
>   #include<sys/socket.h>
>   #include<netdb.h>
>   #include<resolv.h>
> +#include<gweb/gresolv.h>
>
>   #include<glib.h>
>
> @@ -138,6 +139,7 @@ struct cache_data {
>
>   struct cache_entry {
>   	char *key;
> +	int want_refresh;
>   	int hits;
>   	struct cache_data *ipv4;
>   	struct cache_data *ipv6;
> @@ -183,6 +185,7 @@ static GSList *request_list = NULL;
>   static GSList *request_pending_list = NULL;
>   static guint16 request_id = 0x0000;
>   static GHashTable *listener_table = NULL;
> +static time_t next_refresh;
>
>   static int protocol_offset(int protocol)
>   {
> @@ -263,6 +266,48 @@ static struct server_data *find_server(const char *interface,
>   	return NULL;
>   }
>
> +/* we can keep using the same resolve's */
> +static GResolv *ipv4_resolve;
> +static GResolv *ipv6_resolve;

- Could we use the functions in dnsproxy.c directly instead of going 
through GResolv? Although it might be cleaner to do the query via 
GResolv, anyway just an idea.


> +
> +static void dummy_resolve_func(GResolvResultStatus status,
> +                                        char **results, gpointer user_data)
> +{
> +}
> +
> +/*
> + * Refresh a DNS entry, but also age the hit count a bit */
> +static void refresh_dns_entry(struct cache_entry *entry, char *name)
> +{	
> +	int age = 1;
> +	if (!ipv4_resolve) {

- not connman style, better ipv4_resolve == NULL

> +		ipv4_resolve = g_resolv_new(0);
> +		g_resolv_set_address_family(ipv4_resolve, AF_INET);
> +		g_resolv_add_nameserver(ipv4_resolve, "127.0.0.1", 53, 0);
> +	}
> +	if (!ipv6_resolve) {

- ditto

> +		ipv6_resolve = g_resolv_new(0);
> +		g_resolv_set_address_family(ipv6_resolve, AF_INET6);
> +		g_resolv_add_nameserver(ipv6_resolve, "127.0.0.1", 53, 0);
> +	}
> +	
> +	if (!entry->ipv4) {

- ditto

> +		DBG("Refresing A record for %s", name);
> +		g_resolv_lookup_hostname(ipv4_resolve, name, dummy_resolve_func, NULL);

- too long line

> +		age = 4;
> +	}
> +	if (!entry->ipv6) {

- style

> +		DBG("Refresing AAAA record for %s", name);
> +		g_resolv_lookup_hostname(ipv6_resolve, name, dummy_resolve_func, NULL);

- line len

> +		age = 4;
> +	}
> +	entry->hits -= age;
> +	if (entry->hits<  0)
> +		entry->hits = 0;
> +}
> +
> +
> +
>   static int dns_name_length(unsigned char *buf)
>   {
>   	if ((buf[0]&  196) == 196) /* compressed name */

- this could be written like this
	if ((buf[0] & NS_CMPRSFLGS) == NS_CMPRSFLGS)

- I just realized that there is a bug in get_name() that uses 
NS_CMPRSFLGS, I need to send a patch about that one.

> @@ -529,11 +574,18 @@ static void cache_enforce_validity(struct cache_entry *entry)
>   	}
>   }
>
> -
>   static uint16_t cache_check_validity(char *question, uint16_t type,
>   				struct cache_entry *entry)
>   {
>   	time_t current_time = time(0);
> +	int want_refresh = 0;
> +
> +	/*
> +	 * if we have a popular entry, we want a refresh instead of
> +	 * total destruction of the entry.
> +	 */
> +	if (entry->hits>  2)
> +		want_refresh = 1;
>
>   	cache_enforce_validity(entry);
>
> @@ -543,14 +595,17 @@ static uint16_t cache_check_validity(char *question, uint16_t type,
>   			DBG("cache %s \"%s\" type A",
>   				entry->ipv4 ? "timeout" : "entry missing", question);
>
> +			if (want_refresh)
> +				entry->want_refresh = 1;
> +
>   			/*
>   			 * We do not remove cache entry if there is still
>   			 * valid IPv6 entry found in the cache.
>   			 */
> -			if (cache_check_is_valid(entry->ipv6, current_time) == FALSE)
> +			if (cache_check_is_valid(entry->ipv6, current_time) == FALSE&&  !want_refresh) {

- too long line
- also !want_refresh is not connman style so probably want_refresh == 
FALSE would be better here

>   				g_hash_table_remove(cache, question);
> -
> -			type = 0;
> +				type = 0;
> +			}
>   		}
>   		break;
>
> @@ -559,10 +614,13 @@ static uint16_t cache_check_validity(char *question, uint16_t type,
>   			DBG("cache %s \"%s\" type AAAA",
>   				entry->ipv6 ? "timeout" : "entry missing", question);
>
> -			if (cache_check_is_valid(entry->ipv4, current_time) == FALSE)
> -				g_hash_table_remove(cache, question);
> +			if (want_refresh)
> +				entry->want_refresh = 1;
>
> -			type = 0;
> +			if (cache_check_is_valid(entry->ipv4, current_time) == FALSE&&  !want_refresh) {

- ditto

> +				g_hash_table_remove(cache, question);
> +				type = 0;
> +			}
>   		}
>   		break;
>   	}
> @@ -1021,6 +1079,10 @@ static gboolean cache_invalidate_entry(gpointer key, gpointer value,
>   	/* first, delete any expired elements */
>   	cache_enforce_validity(entry);
>
> +	/* if anything is not expired, mark the entry for refresh */
> +	if (entry->hits>  0&&  (entry->ipv4 || entry->ipv6))
> +		entry->want_refresh = 1;
> +
>   	/* delete the cached data */
>   	if (entry->ipv4) {
>   		g_free(entry->ipv4->data);
> @@ -1034,7 +1096,11 @@ static gboolean cache_invalidate_entry(gpointer key, gpointer value,
>   		entry->ipv6 = NULL;
>   	}
>
> -	return TRUE;
> +	/* keep the entry if we want it refreshed, delete it otherwise */
> +	if (entry->want_refresh)
> +		return FALSE;
> +	else
> +		return TRUE;
>   }
>
>   /*
> @@ -1050,6 +1116,51 @@ static void cache_invalidate(void)
>   						NULL);
>   }
>
> +static void cache_refresh_entry(struct cache_entry *entry)
> +{
> +
> +	cache_enforce_validity(entry);
> +
> +	if (entry->hits>  2&&  entry->ipv4 == NULL)
> +		entry->want_refresh = 1;
> +	if (entry->hits>  2&&  entry->ipv6 == NULL)
> +		entry->want_refresh = 1;
> +
> +	if (entry->want_refresh) {
> +		char *c;
> +		char dns_name[NS_MAXDNAME + 1];
> +		entry->want_refresh = 0;
> +
> +		/* turn a DNS name into a hostname with dots */
> +		strncpy(dns_name, entry->key, NS_MAXDNAME);
> +		c = dns_name;
> +		while (c&&  *c) {
> +			int jump;
> +			jump = *c;
> +			*c = '.';
> +			c += jump + 1;
> +		}
> +		DBG("Refreshing %s\n", dns_name);
> +		/* then refresh the hostname */
> +		refresh_dns_entry(entry,&dns_name[1]);
> +	}
> +}
> +
> +static void cache_refresh_iterator(gpointer key, gpointer value,
> +					gpointer user_data)
> +{
> +	struct cache_entry *entry = value;
> +
> +	cache_refresh_entry(entry);

- could we add the cache_refresh_entry() functionality here as now there 
is only one liner in this func


> +}
> +
> +static void cache_refresh(void)
> +{
> +	DBG("Executing a cache refresh\n");

- I think this DBG() is useless here as it is printed many times and we 
might not do the cache refresh even thou the DBG() says we are doing it. 
The debug print in cache_refresh_entry() is enough imho.


> +	g_hash_table_foreach(cache, cache_refresh_iterator, NULL);
> +}
> +
> +
>
>   static int reply_query_type(unsigned char *msg, int len)
>   {
> @@ -1096,6 +1207,15 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
>   			return 0;
>   	}
>
> +	current_time = time(0);
> +
> +	/* don't do a cache refresh more than twice a minute */
> +	if (next_refresh<  current_time) {
> +		cache_refresh();
> +		next_refresh = current_time + 30;
> +	}
> +
> +
>   	/* Continue only if response code is 0 (=ok) */
>   	if (msg[3]&  0x0f)
>   		return 0;
> @@ -1143,7 +1263,6 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
>   		return 0;
>
>   	qlen = strlen(question);
> -	current_time = time(0);
>
>   	/*
>   	 * If the cache contains already data, check if the
> @@ -1166,6 +1285,7 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
>
>   		entry->key = g_strdup(question);
>   		entry->ipv4 = entry->ipv6 = NULL;
> +		entry->want_refresh = 0;
>   		entry->hits = 0;
>
>   		if (type == 1)
> @@ -2029,6 +2149,7 @@ static void dnsproxy_offline_mode(connman_bool_t enabled)
>   			connman_info("Enabling DNS server %s", data->server);
>   			data->enabled = TRUE;
>   			cache_invalidate();
> +			cache_refresh();
>   		} else {
>   			connman_info("Disabling DNS server %s", data->server);
>   			data->enabled = FALSE;
> @@ -2070,6 +2191,7 @@ static void dnsproxy_default_changed(struct connman_service *service)
>   	}
>
>   	g_free(interface);
> +	cache_refresh();
>   }
>
>   static struct connman_notifier dnsproxy_notifier = {


Cheers,
Jukka


More information about the connman mailing list