[CRIU] [PATCH criu v5 7/7] optimization: do not restore configs for devices if value is default

[Pavel Tikhomirov]

On 04/07/2015 09:16 PM, Pavel Emelyanov wrote:
> On 03/31/2015 10:51 AM, Pavel Tikhomirov wrote:
>> because namespace default options are set before devices creation,
>> devices will gain default options(except lo).
>
> OK, modulo two comments in this patch I like the set :)
> Plz, fix the comments and I'll apply one.
>
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov at parallels.com>
>> ---
>>   net.c | 51 +++++++++++++++++++++++++++++++++------------------
>>   1 file changed, 33 insertions(+), 18 deletions(-)
>>
>> diff --git a/net.c b/net.c
>> index e38cf78..0a0043c 100644
>> --- a/net.c
>> +++ b/net.c
>> @@ -85,20 +85,29 @@ char *devconfs[] = {
>>   #define NET_CONF_PATH "net/ipv4/conf"
>>   #define MAX_CONF_OPT_PATH IFNAMSIZ+50
>>
>> -static int ipv4_conf_op(char *tgt, int *conf, int op)
>> +static int ipv4_conf_op(char *tgt, int *conf, int op, NetnsEntry **netns)
>>   {
>> -	int i;
>> +	int i, shift = 0;
>>   	int ret;
>>   	struct sysctl_req req[ARRAY_SIZE(devconfs) + 1];
>>   	char path[ARRAY_SIZE(devconfs)][MAX_CONF_OPT_PATH];
>>
>>   	for (i = 0; devconfs[i]; i++) {
>> +		/*
>> +		 * If dev conf value is the same as default skip restoring it
>> +		 */
>> +		if (netns && conf[i] == (*netns)->def_conf[i]) {
>> +			shift++;
>> +			pr_debug("DEBUG Skip %s/%s, val =%d\n", tgt, devconfs[i], conf[i]);
>> +			continue;
>> +		}
>> +
>>   		sprintf(path[i], "%s/%s/%s", NET_CONF_PATH, tgt, devconfs[i]);
>> -		req[i].name = path[i];
>> -		req[i].arg = &conf[i];
>> -		req[i].type = CTL_32;
>> +		req[i - shift].name = path[i];
>
> This i - shift is hard to parse (by eyes). Better introduce the "int ri" variable,
> use it as index in req[] array and increment one once you put new entry in.

OK

>
>> +		req[i - shift].arg = &conf[i];
>> +		req[i - shift].type = CTL_32;
>>   	}
>> -	req[i].name = NULL;
>> +	req[i - shift].name = NULL;
>>
>>   	ret = sysctl_op(req, op);
>>   	if (ret < 0) {
>
>> @@ -470,7 +479,12 @@ static int restore_links(int pid)
>>   		}
>>
>>   		if (nde->conf) {
>> -			ret = ipv4_conf_op(nde->name, nde->conf, CTL_WRITE);
>> +			/*
>> +			 * lo is created with namespace and before default is set
>> +			 * so we cant optimize its restore
>
> I don't get what the optimization is. If the ret == 0 (it's lo) then we
> pass NULL and make all devconfs to get restored.

We optimize restore of every other device except lo, because lo is a 
special device and it won't get our default values, but it will get 
default values of newly created namespace as they are created together.

(Every device gets default configuration when it is created.)

>
>> +			 */
>> +			ret = strcmp(nde->name, "lo");
>
> Loopback should be detected by nde->type == ND_TYPE__LOOPBACK, not by name.

OK

>
>> +			ret = ipv4_conf_op(nde->name, nde->conf, CTL_WRITE, ret == 0 ? NULL : netns);
>>   			if (!ret)
>>   				return ret;
>>   		}
>

-- 
Best regards, Tikhomirov Pavel
Junior Software Developer, Odin.