On Sun, Dec 13, 2020 at 12:14:19AM +0000, Vladimir Oltean wrote:
> On Sun, Dec 13, 2020 at 01:08:55AM +0100, Andrew Lunn wrote:
> > > > And you need some way to cleanup the allocated memory when the commit
> > > > never happens because some other layer has said No!
> > >
> > > So this would be a fatal problem with the switchdev transactional model
> > > if I am not misunderstanding it. On one hand there's this nice, bubbly
> > > idea that you should preallocate memory in the prepare phase, so that
> > > there's one reason less to fail at commit time. But on the other hand,
> > > if "the commit phase might never happen" is even a remove possibility,
> > > all of that goes to trash - how are you even supposed to free the
> > > preallocated memory.
> >
> > It can definitely happen, that commit is never called:
> >
> > static int switchdev_port_obj_add_now(struct net_device *dev,
> > const struct switchdev_obj *obj,
> > struct netlink_ext_ack *extack)
> > {
> >
> > /* 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
> > * should reserve resources needed for the commit phase here,
> > * but should not commit the obj.
> > */
> >
> > trans.ph_prepare = true;
> > err = switchdev_port_obj_notify(SWITCHDEV_PORT_OBJ_ADD,
> > dev, obj, &trans, extack);
> > if (err)
> > return err;
> >
> > /* Phase II: commit obj add. This cannot fail as a fault
> > * of driver/device. If it does, it's a bug in the driver/device
> > * because the driver said everythings was OK in phase I.
> > */
> >
> > trans.ph_prepare = false;
> > err = switchdev_port_obj_notify(SWITCHDEV_PORT_OBJ_ADD,
> > dev, obj, &trans, extack);
> > WARN(err, "%s: Commit of object (id=%d) failed.\n", dev->name,
> > obj->id);
> >
> > return err;
> >
> > So if any notifier returns an error during prepare, the commit is
> > never called.
> >
> > So the memory you allocated and added to the list may never get
> > used. Its refcount stays zero. Which is why i suggested making the
> > MDB remove call do a general garbage collect. It is not perfect, the
> > cleanup could be deferred a long time, but is should get removed
> > eventually.
>
> What would the garbage collection look like?
struct dsa_host_addr *a;
list_for_each_entry_safe(a, addr_list, list)
if (refcount_read(&a->refcount) == 0) {
list_del(&a->list);
free(a);
}
}
Andrew
