netdev - Re: [PATCH net-next v3 02/11] devlink: Add callback to query for snapshot id before snapshot create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <468b1859-a618-44c9-2567-1dc0bfbf20a6@mellanox.com>
Date:   Sun, 15 Jul 2018 10:43:03 +0300
From:   Alex Vesker <valex@...lanox.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     <netdev@...r.kernel.org>, <jiri@...lanox.com>, <dsahern@...il.com>,
        <andrew@...n.ch>, <rahul.lakkireddy@...lsio.com>
Subject: Re: [PATCH net-next v3 02/11] devlink: Add callback to query for
 snapshot id before snapshot create



On 7/13/2018 3:51 AM, Jakub Kicinski wrote:
> On Thu, 12 Jul 2018 15:13:09 +0300, Alex Vesker wrote:
>> To restrict the driver with the snapshot ID selection a new callback
>> is introduced for the driver to get the snapshot ID before creating
>> a new snapshot. This will also allow giving the same ID for multiple
>> snapshots taken of different regions on the same time.
> I'm not in position to criticize other people's commit messages :), but
> I find this one hard to parse.  I think what you meant to say is that
> you add a helper for numbering the snapshot per-devlink instance.
> There is no callback to be seen here.  You *prevent* from giving the
> same ID to multiple snapshot even if they are from different regions.
Let me try to clarify,
The idea is to have a simple helper function that assigns IDs to provide 
a more complete
API, an example use case is when you want to add a new snapshot to 
multiple regions
from the same trigger, then it should be called once to get an ID, this 
ID should be used
on all new snapshots.

>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index cac8561..6c92ddd 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -4193,6 +4193,27 @@ void devlink_region_destroy(struct devlink_region *region)
>>   }
>>   EXPORT_SYMBOL_GPL(devlink_region_destroy);
>>   
>> +/**
>> + *	devlink_region_shapshot_id_get - get snapshot ID
>> + *
>> + *	This callback should be called when adding a new snapshot,
>> + *	Driver should use the same id for multiple snapshots taken
>> + *	on multiple regions at the same time/by the same trigger.
>> + *
>> + *	@devlink: devlink
>> + */
>> +u32 devlink_region_shapshot_id_get(struct devlink *devlink)
>> +{
>> +	u32 id;
>> +
>> +	mutex_lock(&devlink->lock);
>> +	id = ++devlink->snapshot_id;
> Any reason not to use an IDA?  The reuse may seem unlikely, OTOH IDA
> isn't going to cost much, so why risk it...
As you mentioned more than u32_max_value snapshots doesn't sound likely.
New snapshots will be created, old snapshots should be deleted by the user
a wrap around sounds unlikely. Let me think about it some more, might send a
patch that changes to IDA.

>> +	mutex_unlock(&devlink->lock);
>> +
>> +	return id;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_region_shapshot_id_get);
> Sorry for only spotting this now.