Date:   Thu, 6 Apr 2023 10:18:36 -0600
From:   Max Georgiev <glipus@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     Jakub Kicinski <kuba@...nel.org>, kory.maincent@...tlin.com,
        netdev@...r.kernel.org, maxime.chevallier@...tlin.com,
        vadim.fedorenko@...ux.dev, richardcochran@...il.com,
        gerhard@...leder-embedded.com
Subject: Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

On Thu, Apr 6, 2023 at 9:02 AM Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> On Thu, Apr 06, 2023 at 12:21:37AM -0600, Max Georgiev wrote:
> > I tried my best to follow the discussion, and convert it to compilable code.
> > Here is what I have in mind for generic_hwtstamp_get_lower():
> >
> > int generic_hwtstamp_get_lower(struct net_dev *dev,
> >                            struct kernel_hwtstamp_config *kernel_cfg,
> >                            struct netlink_ext_ack *extack)
> > {
> >         const struct net_device_ops *ops = dev->netdev_ops;
> >         struct hwtstamp_config cfg;
> >         int err;
> >
> >         if (!netif_device_present(dev))
> >                 return -ENODEV;
> >
> >         if (ops->ndo_hwtstamp_get)
> >                 return ops->ndo_hwtstamp_get(dev, cfg, extack);
> >
> >         if (!cfg->ifr)
> >                 return -EOPNOTSUPP;
> >
> >         err = dev_eth_ioctl(dev, cfg->ifr, SIOCGHWTSTAMP);
> >         if (err)
> >             return err;
> >
> >         if (copy_from_user(&cfg, cfg->ifr->ifr_data, sizeof(cfg)))
> >                 return -EFAULT;
> >
> >         hwtstamp_config_to_kernel(kernel_cfg, &cfg);
> >
> >         return 0;
> > }
>
> Side note, it doesn't look like this code is particularly compilable
> either - "cfg" is used in some places instead of "kernel_cfg".

That's true, this code wouldn't compile. I copied it here to
illustrate the potentially concerning logic.
Thank you for pointing this out though!

>
> >
> > It looks like there is a possibility that the returned hwtstamp_config structure
> > will be copied twice to ifr and copied once from ifr on the return path
> > in case if the underlying driver does not implement ndo_hwtstamp_get():
> > - the underlying driver calls copy_to_user() inside its ndo_eth_ioctl()
> >   implementation to return the data to generic_hwtstamp_get_lower();
> > - then generic_hwtstamp_get_lower() calls copy_from_user() to copy it
> >   back out of the ifr to kernel_hwtstamp_config structure;
> > - then dev_get_hwtstamp() calls copy_to_user() again to update
> >   the same ifr with the same data the ifr already contains.
> >
> > Should we consider this acceptable?
>
> Thanks for laying this out. I guess with a table it's going to be
> clearer, so to summarize, I believe this is the status:
>
> Assuming we convert *vlan to ndo_hwtstamp_set():
>
> ===================
>
> If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev
> driver uses ndo_eth_ioctl(), we have:
> - one copy_from_user() in dev_set_hwtstamp()
> - one copy_from_user() in the real_dev's ndo_eth_ioctl()
> - one copy_to_user() in the real_dev's ndo_eth_ioctl()
> - one copy_from_user() in generic_hwtstamp_get_lower()
> - one copy_to_user() in dev_set_hwtstamp()
>
> If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev
> is converted too, we have:
> - one copy_from_user() in dev_set_hwtstamp()
> - one copy_to_user() in dev_set_hwtstamp()
>
> ===================
>
> Assuming we don't convert *vlan to ndo_hwtstamp_set():
>
> ===================
>
> If the vlan driver isn't converted to ndo_hwtstamp_set() and the
> real_dev driver isn't converted either, we have:
> - one copy_from_user() in dev_set_hwtstamp()
> - one copy_from_user() in the real_dev's ndo_eth_ioctl()
> - one copy_to_user() in the real_dev's ndo_eth_ioctl()
>
> If the vlan driver isn't converted to ndo_hwtstamp_set(), but the
> real_dev driver is, we have:
> - one copy_from_user() in dev_set_hwtstamp()
> - one copy_from_user() in the vlan's ndo_eth_ioctl()
> - one copy_to_user() in the vlan's ndo_eth_ioctl()
>
> ===================
>
> So between converting and not converting the *vlans to ndo_hwtstamp_set(),
> the worst case is going to be worse (with a mix of new API in *vlan and
> old API in real_dev) and the best case is going to be better (with new
> API in both *vlan and real_dev). OTOH, with old API in *vlan, the number
> of copies to/from the user buffer is going to be constant at 3, which is
> not the best, not the worst.
>
> I guess the data indicates that we should convert the *vlans to
> ndo_hwtstamp_set() at the very end of the process, and for now, just
> make them compatible with a real_dev that uses the new API?
>
> Note that I haven't done the math for the "get" operation yet, but I
> believe it to be similar.

Thank you for putting this overhead tracking table together!

Here is my guess on how this accounting would look like
for the "get" codepath:

Assuming we convert *vlan to ndo_hwtstamp_set():

===================

If the vlan driver is converted to ndo_hwtstamp_get() and the real_dev
driver uses ndo_eth_ioctl(), we have:
- one copy_to_user() in the real_dev's ndo_eth_ioctl()
- one copy_from_user() in generic_hwtstamp_get_lower()
- one copy_to_user() in dev_get_hwtstamp()

If the vlan driver is converted to ndo_hwtstamp_get() and the real_dev
is converted too, we have:
- one copy_to_user() in dev_get_hwtstamp()

===================

Assuming we don't convert *vlan to ndo_hwtstamp_get():

===================

If the vlan driver isn't converted to ndo_hwtstamp_get() and the
real_dev driver isn't converted either, we have:
- one copy_to_user() in the real_dev's ndo_eth_ioctl()

If the vlan driver isn't converted to ndo_hwtstamp_get(), but the
real_dev driver is, we have:
- one copy_to_user() in the vlan's ndo_eth_ioctl()

===================

Adding real_dev->ndo_hwtstamp_get/set() support to vlan_eth_ioctl()
and holding back with ndo_hwtstamp_get/set() implementation in vlan
code looks like a winner again.

If I may, there are other ways to work around this inefficiency.
Since kernel_hwtstamp_config was meant to be easily extendable,
we can abuse it and add a flag field there. One of the flag values
can indicate that the operation result structure was already copied
to kernel_config->ifr by the function that received this kernel_config
instance as a parameter, and that the content of the
hwtstamp_config-related fields in kernel_config structure must
be ignored when the function returns. It would complicate the
implementation logic, but we'd avoid some unnecessary copy
operations while converting *vlan components to the newer interface.
Would it be a completely unreasonable approach?

Powered by blists - more mailing lists