netdev - Re: [patch net-next v2 02/10] switchdev: introduce transaction item queue for attr_set and obj

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]

Date:	Wed, 23 Sep 2015 17:56:12 +0200
From:	Jiri Pirko <jiri@...nulli.us>
To:	Scott Feldman <sfeldma@...il.com>
Cc:	Netdev <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Ido Schimmel <idosch@...lanox.com>, eladr@...lanox.com,
	Florian Fainelli <f.fainelli@...il.com>,
	Guenter Roeck <linux@...ck-us.net>,
	Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
	"Rosen, Rami" <rami.rosen@...el.com>,
	Roopa Prabhu <roopa@...ulusnetworks.com>,
	Premkumar Jonnala <pjonnala@...adcom.com>,
	"andrew@...n.ch" <andrew@...n.ch>,
	Andy Gospodarek <gospo@...ulusnetworks.com>,
	Jiri Pirko <jiri@...lanox.com>
Subject: Re: [patch net-next v2 02/10] switchdev: introduce transaction item
 queue for attr_set and obj_add

Wed, Sep 23, 2015 at 05:46:44PM CEST, sfeldma@...il.com wrote:
>On Wed, Sep 23, 2015 at 12:57 AM, Jiri Pirko <jiri@...nulli.us> wrote:
>> From: Jiri Pirko <jiri@...lanox.com>
>>
>> Now, the memory allocation in prepare/commit state is done separatelly
>> in each driver (rocker). Introduce the similar mechanism in generic
>> switchdev code, in form of queue. That can be used not only for memory
>> allocations, but also for different items. Commit/abort item destruction
>> is handled as well.
>>
>> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>> ---
>> v1-v2:
>> - added comment blocks to exported functions as suggested by Scott
>> - made switchdev_trans_init and switchdev_trans_items_destroy static
>> - added documentation for this to switchdev.txt as suggested by Scott
>
>[cut]
>
>> @@ -232,10 +304,13 @@ static int __switchdev_port_obj_add(struct net_device *dev,
>>   */
>>  int switchdev_port_obj_add(struct net_device *dev, struct switchdev_obj *obj)
>>  {
>> +       struct switchdev_trans trans;
>>         int err;
>>
>>         ASSERT_RTNL();
>>
>> +       switchdev_trans_init(&trans);
>> +
>>         /* Phase I: prepare for obj add. Driver/device should fail
>>          * here if there are going to be issues in the commit phase,
>>          * such as lack of resources or support.  The driver/device
>> @@ -244,7 +319,7 @@ int switchdev_port_obj_add(struct net_device *dev, struct switchdev_obj *obj)
>>          */
>>
>>         obj->trans_ph = SWITCHDEV_TRANS_PREPARE;
>> -       err = __switchdev_port_obj_add(dev, obj);
>> +       err = __switchdev_port_obj_add(dev, obj, &trans);
>>         if (err) {
>>                 /* Prepare phase failed: abort the transaction.  Any
>>                  * resources reserved in the prepare phase are
>> @@ -253,7 +328,8 @@ int switchdev_port_obj_add(struct net_device *dev, struct switchdev_obj *obj)
>>
>>                 if (err != -EOPNOTSUPP) {
>>                         obj->trans_ph = SWITCHDEV_TRANS_ABORT;
>> -                       __switchdev_port_obj_add(dev, obj);
>> +                       __switchdev_port_obj_add(dev, obj, &trans);
>> +                       switchdev_trans_items_destroy(&trans);
>>                 }
>>
>>                 return err;
>> @@ -265,8 +341,9 @@ int switchdev_port_obj_add(struct net_device *dev, struct switchdev_obj *obj)
>>          */
>>
>>         obj->trans_ph = SWITCHDEV_TRANS_COMMIT;
>> -       err = __switchdev_port_obj_add(dev, obj);
>> +       err = __switchdev_port_obj_add(dev, obj, &trans);
>>         WARN(err, "%s: Commit of object (id=%d) failed.\n", dev->name, obj->id);
>> +       switchdev_trans_items_destroy(&trans);
>
>Before this patchset, rocker would BUG_ON that the list was empty
>after commit.  This was really helpful in finding bugs in the driver
>as it meant the commit and prepare phases didn't follow the same code
>path.  (Everything the driver queued in prepare was dequeued in
>commit, so the list should be empty after commit).
>
>Should this last switchdev_trans_items_destroy(&trans) be replaced with:
>
>    BUG_ON(!list_empty(&trans.item_list));

Sounds reasonable. Will change this.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html