From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> To: Robin Murphy <robin.murphy@arm.com> Cc: sgoutham@cavium.com, sunil.kovvuri@gmail.com, rric@kernel.org, linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, davem@davemloft.net, dnelson@redhat.com, Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>, gustavo@embeddedor.com Subject: Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array Date: Fri, 6 Apr 2018 05:07:42 -0700 [thread overview] Message-ID: <20180406120742.GB14761@localhost.localdomain> (raw) In-Reply-To: <32d75865-6407-9e9e-a78d-38ba761b2575@arm.com> 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++; > > } > > } > >
WARNING: multiple messages have this Message-ID (diff)
From: Vadim.Lomovtsev@caviumnetworks.com (Vadim Lomovtsev) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2] net: thunderx: rework mac addresses list to u64 array Date: Fri, 6 Apr 2018 05:07:42 -0700 [thread overview] Message-ID: <20180406120742.GB14761@localhost.localdomain> (raw) In-Reply-To: <32d75865-6407-9e9e-a78d-38ba761b2575@arm.com> 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++; > > } > > } > >
next prev parent reply other threads:[~2018-04-06 12:07 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-05 14:57 [PATCH] net: thunderx: rework mac addresses list to u64 array Vadim Lomovtsev 2018-04-05 14:57 ` Vadim Lomovtsev 2018-04-05 15:00 ` Vadim Lomovtsev 2018-04-05 15:00 ` Vadim Lomovtsev 2018-04-05 15:07 ` Christoph Hellwig 2018-04-05 15:07 ` Christoph Hellwig 2018-04-05 16:07 ` Vadim Lomovtsev 2018-04-05 16:07 ` Vadim Lomovtsev 2018-04-05 16:15 ` Christoph Hellwig 2018-04-05 16:15 ` Christoph Hellwig 2018-04-06 11:14 ` [PATCH v2] " Vadim Lomovtsev 2018-04-06 11:14 ` Vadim Lomovtsev 2018-04-06 11:36 ` Gustavo A. R. Silva 2018-04-06 11:36 ` Gustavo A. R. Silva 2018-04-06 11:43 ` Vadim Lomovtsev 2018-04-06 11:43 ` Vadim Lomovtsev 2018-04-06 11:47 ` Gustavo A. R. Silva 2018-04-06 11:47 ` Gustavo A. R. Silva 2018-04-06 11:53 ` Vadim Lomovtsev 2018-04-06 11:53 ` Vadim Lomovtsev 2018-04-06 11:48 ` Robin Murphy 2018-04-06 11:48 ` Robin Murphy 2018-04-06 12:07 ` Vadim Lomovtsev [this message] 2018-04-06 12:07 ` Vadim Lomovtsev 2018-04-06 14:04 ` [PATCH v3] " Vadim Lomovtsev 2018-04-06 14:04 ` Vadim Lomovtsev 2018-04-06 15:16 ` Vadim Lomovtsev 2018-04-06 15:16 ` Vadim Lomovtsev 2018-04-06 19:53 ` [PATCH v4] " Vadim Lomovtsev 2018-04-06 19:53 ` Vadim Lomovtsev 2018-04-08 16:42 ` David Miller 2018-04-08 16:42 ` David Miller 2018-04-09 9:42 ` Vadim Lomovtsev 2018-04-09 9:42 ` Vadim Lomovtsev 2018-04-09 13:24 ` [PATCH v5] " Vadim Lomovtsev 2018-04-09 13:24 ` Vadim Lomovtsev 2018-04-09 15:00 ` David Miller 2018-04-09 15:00 ` David Miller 2018-04-06 15:06 ` [PATCH v2] " David Miller 2018-04-06 15:06 ` David Miller 2018-04-06 15:14 ` Vadim Lomovtsev 2018-04-06 15:14 ` Vadim Lomovtsev
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180406120742.GB14761@localhost.localdomain \ --to=vadim.lomovtsev@caviumnetworks.com \ --cc=Vadim.Lomovtsev@cavium.com \ --cc=davem@davemloft.net \ --cc=dnelson@redhat.com \ --cc=gustavo@embeddedor.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=robin.murphy@arm.com \ --cc=rric@kernel.org \ --cc=sgoutham@cavium.com \ --cc=sunil.kovvuri@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.