lkml.org 
[lkml]   [2018]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
    Hi Robin,

    On Fri, Apr 06, 2018 at 12:48:40PM +0100, Robin Murphy wrote:
    > On 06/04/18 12:14, Vadim Lomovtsev wrote:
    > > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
    > >
    > > It is too expensive to pass u64 values via linked list, instead
    > > allocate array for them by overall number of mac addresses from netdev.
    > >
    > > This eventually removes multiple kmalloc() calls, aviod memory
    > > fragmentation and allow to put single null check on kmalloc
    > > return value in order to prevent a potential null pointer dereference.
    > >
    > > Addresses-Coverity-ID: 1467429 ("Dereference null return value")
    > > Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
    > > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
    > > ---
    > > Changes from v1 to v2:
    > > - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
    > >
    > > ---
    > > drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
    > > drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
    > > 2 files changed, 11 insertions(+), 24 deletions(-)
    > >
    > > diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
    > > index 5fc46c5a4f36..448d1fafc827 100644
    > > --- a/drivers/net/ethernet/cavium/thunder/nic.h
    > > +++ b/drivers/net/ethernet/cavium/thunder/nic.h
    > > @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
    > > struct cavium_ptp;
    > > -struct xcast_addr {
    > > - struct list_head list;
    > > - u64 addr;
    > > -};
    > > -
    > > struct xcast_addr_list {
    > > - struct list_head list;
    > > int count;
    > > + u64 mc[];
    > > };
    > > struct nicvf_work {
    > > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
    > > index 1e9a31fef729..a26d8bc92e01 100644
    > > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
    > > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
    > > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
    > > work.work);
    > > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
    > > union nic_mbx mbx = {};
    > > - struct xcast_addr *xaddr, *next;
    > > + u8 idx = 0;
    > > if (!vf_work)
    > > return;
    > > @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
    > > /* check if we have any specific MACs to be added to PF DMAC filter */
    > > if (vf_work->mc) {
    > > /* now go through kernel list of MACs and add them one by one */
    > > - list_for_each_entry_safe(xaddr, next,
    > > - &vf_work->mc->list, list) {
    > > + for (idx = 0; idx < vf_work->mc->count; idx++) {
    > > mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
    > > - mbx.xcast.data.mac = xaddr->addr;
    > > + mbx.xcast.data.mac = vf_work->mc->mc[idx];
    > > nicvf_send_msg_to_pf(nic, &mbx);
    > > -
    > > - /* after receiving ACK from PF release memory */
    > > - list_del(&xaddr->list);
    > > - kfree(xaddr);
    > > - vf_work->mc->count--;
    > > }
    > > kfree(vf_work->mc);
    > > }
    > > @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
    > > mode |= BGX_XCAST_MCAST_FILTER;
    > > /* here we need to copy mc addrs */
    > > if (netdev_mc_count(netdev)) {
    > > - struct xcast_addr *xaddr;
    > > -
    > > - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
    > > - INIT_LIST_HEAD(&mc_list->list);
    > > + mc_list = kmalloc(sizeof(*mc_list) +
    > > + sizeof(u64) * netdev_mc_count(netdev),
    >
    > FWIW if you really wanted to disambiguate that it's a structure and not just
    > an array being allocated, then it could be expressed without explicit
    > arithmetic:
    >
    > size = offsetof(typeof(*mc_list), mc[netdev_mc_count(netdev)]);
    >
    > but that's probably just a matter of personal preference at this point.
    >
    > Robin.
    >

    Thanks for reviewing it and for hint. From style perspective, I believe,
    I should get rid off direct types names also, and use your suggestion here.

    I'll update patch to v3, test and re-post.
    Thank you for your time.

    WBR,
    Vadim

    > > + GFP_ATOMIC);
    > > + if (unlikely(!mc_list))
    > > + return;
    > > + mc_list->count = 0;
    > > netdev_hw_addr_list_for_each(ha, &netdev->mc) {
    > > - xaddr = kmalloc(sizeof(*xaddr),
    > > - GFP_ATOMIC);
    > > - xaddr->addr =
    > > + mc_list->mc[mc_list->count] =
    > > ether_addr_to_u64(ha->addr);
    > > - list_add_tail(&xaddr->list,
    > > - &mc_list->list);
    > > mc_list->count++;
    > > }
    > > }
    > >

    \
     
     \ /
      Last update: 2018-04-06 14:08    [W:4.579 / U:0.240 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site