Mailing List Archive

On Thu, 2011-03-10 at 09:15 -0800, Stephen Hemminger wrote:
> On Thu, 10 Mar 2011 17:02:33 +0000
> Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > +struct xenvif {
> > + /* Unique identifier for this interface. */
> ...
> > + struct net_device_stats stats;
> > +
>
> There is already a stats struct in net_device in current kernel
> versions, unless there is a compelling reason otherwise
> please use that.
>
> Also, you probably want to implement per-cpu and 64 bit
> stats.

The driver is using a single queue, so I don't see what benefit it would
get from per-cpu stats. At some point it should become multiqueue and
then it should store per-queue stats.

64-bit stats are definitely preferable, but since they're being
maintained on the data path this may require some significant work.
(Ian: see <linux/u64_stats_sync.h> for the canonical way to do this.)
Given that only a relatively few existing drivers do this (I count 13),
I'm not sure we can reasonably demand that a new driver does - yet.

Ben.

--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2011-03-10 at 17:30 +0000, Ben Hutchings wrote:
> On Thu, 2011-03-10 at 09:15 -0800, Stephen Hemminger wrote:
> > On Thu, 10 Mar 2011 17:02:33 +0000
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > +struct xenvif {
> > > + /* Unique identifier for this interface. */
> > ...
> > > + struct net_device_stats stats;
> > > +
> >
> > There is already a stats struct in net_device in current kernel
> > versions, unless there is a compelling reason otherwise
> > please use that.

Will do.

> > Also, you probably want to implement per-cpu and 64 bit
> > stats.
>
> The driver is using a single queue, so I don't see what benefit it would
> get from per-cpu stats. At some point it should become multiqueue and
> then it should store per-queue stats.

Agreed.

> 64-bit stats are definitely preferable, but since they're being
> maintained on the data path this may require some significant work.
> (Ian: see <linux/u64_stats_sync.h> for the canonical way to do this.)
> Given that only a relatively few existing drivers do this (I count 13),
> I'm not sure we can reasonably demand that a new driver does - yet.

Is there an example of a driver which also updates the stats on the
datapath?

If it's ok I'd prefer to defer this change for now though.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2011-03-10 at 17:17 +0000, Ben Hutchings wrote:
> On Thu, 2011-03-10 at 09:14 -0800, Stephen Hemminger wrote:
> > On Thu, 10 Mar 2011 17:02:33 +0000
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >
> > > + /* Flags that must not be set in dev->features */
> > > + int features_disabled;
> >
> > dev->features is unsigned long, so this mask should be as well.
>
> It's u32 now.

That's in net-next only but I think there's no harm in making it u32 in
my netback branch (which is based on a tree which uses unsigned long).
Then it will be correct when merged.

Alternatively I could pull net-next into my branch but I don't think
that's necessary in this case.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, 11 Mar 2011 10:22:55 +0000
Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:

> On Thu, 2011-03-10 at 17:30 +0000, Ben Hutchings wrote:
> > On Thu, 2011-03-10 at 09:15 -0800, Stephen Hemminger wrote:
> > > On Thu, 10 Mar 2011 17:02:33 +0000
> > > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > > +struct xenvif {
> > > > + /* Unique identifier for this interface. */
> > > ...
> > > > + struct net_device_stats stats;
> > > > +
> > >
> > > There is already a stats struct in net_device in current kernel
> > > versions, unless there is a compelling reason otherwise
> > > please use that.
>
> Will do.
>
> > > Also, you probably want to implement per-cpu and 64 bit
> > > stats.
> >
> > The driver is using a single queue, so I don't see what benefit it would
> > get from per-cpu stats. At some point it should become multiqueue and
> > then it should store per-queue stats.
>
> Agreed.
>
> > 64-bit stats are definitely preferable, but since they're being
> > maintained on the data path this may require some significant work.
> > (Ian: see <linux/u64_stats_sync.h> for the canonical way to do this.)
> > Given that only a relatively few existing drivers do this (I count 13),
> > I'm not sure we can reasonably demand that a new driver does - yet.
>
> Is there an example of a driver which also updates the stats on the
> datapath?
>
> If it's ok I'd prefer to defer this change for now though.

Sure, my original comment was just a hint for future.


--

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2011-03-10 at 17:33 +0000, Michał Mirosław wrote:
> 2011/3/10 Ian Campbell <Ian.Campbell@citrix.com>:
> > The following patch is the fourth iteration of the Xen network backend
> > driver for upstream Linux.
> >
> > This driver ("netback") is the host side counterpart to the frontend
> > driver in drivers/net/xen-netfront.c. The PV protocol is also
> > implemented by frontend drivers in other OSes too, such as the BSDs and
> > even Windows.
> [...]
> > --- /dev/null
> > +++ b/drivers/net/xen-netback/common.h
> > @@ -0,0 +1,162 @@
> [...]
> > +struct xenvif {
> [...]
> > + /* Flags that must not be set in dev->features */
> > + int features_disabled;
> > +
> > + /* Frontend feature information. */
> > + u8 can_sg:1;
> > + u8 gso:1;
> > + u8 gso_prefix:1;
> > + u8 csum:1;
> [...]
> > --- /dev/null
> > +++ b/drivers/net/xen-netback/interface.c
> > @@ -0,0 +1,424 @@
> [...]
> > +static int xenvif_change_mtu(struct net_device *dev, int mtu)
> > +{
> > + struct xenvif *vif = netdev_priv(dev);
> > + int max = vif->can_sg ? 65535 - VLAN_ETH_HLEN : ETH_DATA_LEN;
> > +
> > + if (mtu > max)
> > + return -EINVAL;
> > + dev->mtu = mtu;
> > + return 0;
> > +}
> > +
> > +static void xenvif_set_features(struct xenvif *vif)
> > +{
> > + struct net_device *dev = vif->dev;
> > + int features = dev->features;
> > +
> > + if (vif->can_sg)
> > + features |= NETIF_F_SG;
> > + if (vif->gso || vif->gso_prefix)
> > + features |= NETIF_F_TSO;
> > + if (vif->csum)
> > + features |= NETIF_F_IP_CSUM;
> > +
> > + features &= ~(vif->features_disabled);
> > +
> > + if (!(features & NETIF_F_SG) && dev->mtu > ETH_DATA_LEN)
> > + dev->mtu = ETH_DATA_LEN;
> > +
> > + dev->features = features;
> > +}
> > +
> > +static int xenvif_set_tx_csum(struct net_device *dev, u32 data)
> > +{
> > + struct xenvif *vif = netdev_priv(dev);
> > + if (data) {
> > + if (!vif->csum)
> > + return -EOPNOTSUPP;
> > + vif->features_disabled &= ~NETIF_F_IP_CSUM;
> > + } else {
> > + vif->features_disabled |= NETIF_F_IP_CSUM;
> > + }
> > +
> > + xenvif_set_features(vif);
> > + return 0;
> > +}
> > +
> > +static int xenvif_set_sg(struct net_device *dev, u32 data)
> > +{
> > + struct xenvif *vif = netdev_priv(dev);
> > + if (data) {
> > + if (!vif->can_sg)
> > + return -EOPNOTSUPP;
> > + vif->features_disabled &= ~NETIF_F_SG;
> > + } else {
> > + vif->features_disabled |= NETIF_F_SG;
> > + }
> > +
> > + xenvif_set_features(vif);
> > + return 0;
> > +}
> > +
> > +static int xenvif_set_tso(struct net_device *dev, u32 data)
> > +{
> > + struct xenvif *vif = netdev_priv(dev);
> > + if (data) {
> > + if (!vif->gso && !vif->gso_prefix)
> > + return -EOPNOTSUPP;
> > + vif->features_disabled &= ~NETIF_F_TSO;
> > + } else {
> > + vif->features_disabled |= NETIF_F_TSO;
> > + }
> > +
> > + xenvif_set_features(vif);
> > + return 0;
> > +}
> [...]
>
> You could make this simpler by using netdev->hw_features and
> ndo_fix_features/ndo_set_features calls recently introduced in
> net-next.

Thanks, this does look like it could simplify things. Doesn't look like
its use is widespread (even net-next) yet though?

I'll add it to my TODO list for the future.

> I'll reply with a proof-of-concept patch for xen-netfront.
>
> Best Regards,
> Michał Mirosław



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel