netdev - Re: [net-next v2] net: ifb: support ethtools stats

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: <CAMDZJNVYnPHCNa3+6qHgEV3D0LGvRS0ov5bv2=zy+mwgC3oyDg@mail.gmail.com>
Date:   Mon, 29 Nov 2021 12:57:48 +0800
From:   Tonghao Zhang <xiangxia.m.yue@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [net-next v2] net: ifb: support ethtools stats

On Sat, Nov 27, 2021 at 5:42 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Fri, 26 Nov 2021 11:23:05 +0800 xiangxia.m.yue@...il.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@...il.com>
> >
> > With this feature, we can use the ethtools to get tx/rx
> > queues stats. This patch, introduce the ifb_update_q_stats
> > helper to update the queues stats, and ifb_q_stats to simplify
> > the codes. In future, we can add more metrics in ifb_q_stats.
> >
> > Cc: Jakub Kicinski <kuba@...nel.org>
> > Cc: "David S. Miller" <davem@...emloft.net>
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.com>
>
> >  static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev);
> >  static int ifb_open(struct net_device *dev);
> >  static int ifb_close(struct net_device *dev);
> >
> > +static inline void ifb_update_q_stats(struct ifb_q_stats *stats, int len)
>
> Please remove the "inline" keywords, we prefer to leave the choice to
> the compiler (plus it hides "unused function" warnings if the caller
> is ever removed).
>
> > +static void ifb_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
> > +{
> > +     u8 *p = buf;
> > +     int i, j;
> > +
> > +     switch(stringset) {
> > +     case ETH_SS_STATS:
> > +             for (i = 0; i < dev->real_num_rx_queues; i++)
> > +                     for (j = 0; j < IFB_Q_STATS_LEN; j++)
> > +                             ethtool_sprintf(&p, "rx_queue_%u_%.18s",
> > +                                             i, ifb_q_stats_desc[j].desc);
> > +
> > +             for (i = 0; i < dev->real_num_tx_queues; i++)
> > +                     for (j = 0; j < IFB_Q_STATS_LEN; j++)
> > +                             ethtool_sprintf(&p, "tx_queue_%u_%.18s",
> > +                                             i, ifb_q_stats_desc[j].desc);
> > +
> > +             break;
> > +     }
> > +}
> > +
> > +static int ifb_get_sset_count(struct net_device *dev, int sset)
> > +{
> > +     switch (sset) {
> > +     case ETH_SS_STATS:
> > +             return IFB_Q_STATS_LEN * (dev->real_num_rx_queues +
> > +                    dev->real_num_tx_queues);
>
> Needs to align under opening bracket, try checkpatch --strict.
>
> > +     default:
> > +             return -EOPNOTSUPP;
> > +     }
> > +}
> > +
> > +static inline void ifb_fill_stats_data(u64 **data,
> > +                                    struct ifb_q_stats *q_stats)
>
> another inline
Thanks, Jakub
v3 is sent, https://patchwork.kernel.org/project/netdevbpf/patch/20211128014631.43627-1-xiangxia.m.yue@gmail.com/

> The logic itself LGTM.




--
Best regards, Tonghao