Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Troy Kisky]

On 3/4/2016 12:41 AM, Fugang Duan wrote:
> From: Troy Kisky <troy.ki...@boundarydevices.com> Sent: Thursday, March 03, 
> 2016 12:14 AM
>> To: Fugang Duan <fugang.d...@nxp.com>; netdev@vger.kernel.org;
>> da...@davemloft.net; b38...@freescale.com
>> Cc: fabio.este...@freescale.com; l.st...@pengutronix.de; and...@lunn.ch;
>> trem...@gmail.com; li...@arm.linux.org.uk; linux-arm-
>> ker...@lists.infradead.org; l...@boundarydevices.com; shawn...@kernel.org;
>> johan...@sipsolutions.net; stillcompil...@gmail.com;
>> sergei.shtyl...@cogentembedded.com; a...@arndb.de
>> Subject: Re: [PATCH net-next V2 03/16] net: fec: pass txq to
>> fec_enet_tx_queue instead of queue_id
>>
>> On 3/2/2016 8:16 AM, Fugang Duan wrote:
>>> From: Troy Kisky <troy.ki...@boundarydevices.com>  Sent: Thursday,
>>> February 25, 2016 8:37 AM
>>>> To: netdev@vger.kernel.org; da...@davemloft.net;
>> b38...@freescale.com
>>>> Cc: fabio.este...@freescale.com; l.st...@pengutronix.de;
>>>> and...@lunn.ch; trem...@gmail.com; li...@arm.linux.org.uk; linux-arm-
>>>> ker...@lists.infradead.org; l...@boundarydevices.com;
>>>> shawn...@kernel.org; johan...@sipsolutions.net;
>>>> stillcompil...@gmail.com; sergei.shtyl...@cogentembedded.com;
>>>> a...@arndb.de; Troy Kisky <troy.ki...@boundarydevices.com>
>>>> Subject: [PATCH net-next V2 03/16] net: fec: pass txq to
>>>> fec_enet_tx_queue instead of queue_id
>>>>
>>>> queue_id is the qid member of struct bufdesc_prop.
>>>>
>>>> Signed-off-by: Troy Kisky <troy.ki...@boundarydevices.com>
>>>> ---
>>>>  drivers/net/ethernet/freescale/fec_main.c | 17 ++---
>>>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>>> b/drivers/net/ethernet/freescale/fec_main.c
>>>> index 9619b9e..c517194 100644
>>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>>> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private
>>>> *fep, unsigned ts,
>>>>hwtstamps->hwtstamp = ns_to_ktime(ns);  }
>>>>
>>>> -static void
>>>> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>>>> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
>>>> +  struct fec_enet_priv_tx_q *txq)
>>>>  {
>>>> -  struct  fec_enet_private *fep;
>>>>struct bufdesc *bdp;
>>>>unsigned short status;
>>>>struct  sk_buff *skb;
>>>> -  struct fec_enet_priv_tx_q *txq;
>>>>struct netdev_queue *nq;
>>>>int index = 0;
>>>>int entries_free;
>>>>
>>>> -  fep = netdev_priv(ndev);
>>>> -
>>>> -  queue_id = FEC_ENET_GET_QUQUE(queue_id);
>>>> -
>>>> -  txq = fep->tx_queue[queue_id];
>>>>/* get next bdp of dirty_tx */
>>>> -  nq = netdev_get_tx_queue(ndev, queue_id);
>>>> +  nq = netdev_get_tx_queue(ndev, txq->bd.qid);
>>>>bdp = txq->dirty_tx;
>>>>
>>>>/* get next bdp of dirty_tx */
>>>> @@ -1268,11 +1261,13 @@ static void
>>>>  fec_enet_tx(struct net_device *ndev)  {
>>>>struct fec_enet_private *fep = netdev_priv(ndev);
>>>> +  struct fec_enet_priv_tx_q *txq;
>>>>u16 queue_id;
>>>>/* First process class A queue, then Class B and Best Effort queue */
>>>>for_each_set_bit(queue_id, >work_tx, FEC_ENET_MAX_TX_QS)
>> {
>>>>clear_bit(queue_id, >work_tx);
>>>> -  fec_enet_tx_queue(ndev, queue_id);
>>>> +  txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
>>>> +  fec_txq(ndev, fep, txq);
>>>>}
>>>>return;
>>>>  }
>>>> --
>>>> 2.5.0
>>>
>>> The patch should merge with patch#1.
>>>
>>
>>
>> Why ?  That would only hide the change in patch #1.
> 
> Hi Troy Kisky,
> 
> Sorry, I mean patch#2 net: fec: pass rxq to fec_enet_rx_queue instead of 
> queue_id.  It is not necessary to separate them.
> 



I'll happily squash them together, that is easy, and I will.
I don't agree they should be, but it is just not at all important to me.


Sincere thanks for reviewing this series though.

Troy

RE: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Fugang Duan]

From: Troy Kisky <troy.ki...@boundarydevices.com> Sent: Thursday, March 03, 
2016 12:14 AM
> To: Fugang Duan <fugang.d...@nxp.com>; netdev@vger.kernel.org;
> da...@davemloft.net; b38...@freescale.com
> Cc: fabio.este...@freescale.com; l.st...@pengutronix.de; and...@lunn.ch;
> trem...@gmail.com; li...@arm.linux.org.uk; linux-arm-
> ker...@lists.infradead.org; l...@boundarydevices.com; shawn...@kernel.org;
> johan...@sipsolutions.net; stillcompil...@gmail.com;
> sergei.shtyl...@cogentembedded.com; a...@arndb.de
> Subject: Re: [PATCH net-next V2 03/16] net: fec: pass txq to
> fec_enet_tx_queue instead of queue_id
> 
> On 3/2/2016 8:16 AM, Fugang Duan wrote:
> > From: Troy Kisky <troy.ki...@boundarydevices.com>  Sent: Thursday,
> > February 25, 2016 8:37 AM
> >> To: netdev@vger.kernel.org; da...@davemloft.net;
> b38...@freescale.com
> >> Cc: fabio.este...@freescale.com; l.st...@pengutronix.de;
> >> and...@lunn.ch; trem...@gmail.com; li...@arm.linux.org.uk; linux-arm-
> >> ker...@lists.infradead.org; l...@boundarydevices.com;
> >> shawn...@kernel.org; johan...@sipsolutions.net;
> >> stillcompil...@gmail.com; sergei.shtyl...@cogentembedded.com;
> >> a...@arndb.de; Troy Kisky <troy.ki...@boundarydevices.com>
> >> Subject: [PATCH net-next V2 03/16] net: fec: pass txq to
> >> fec_enet_tx_queue instead of queue_id
> >>
> >> queue_id is the qid member of struct bufdesc_prop.
> >>
> >> Signed-off-by: Troy Kisky <troy.ki...@boundarydevices.com>
> >> ---
> >>  drivers/net/ethernet/freescale/fec_main.c | 17 ++---
> >>  1 file changed, 6 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> >> b/drivers/net/ethernet/freescale/fec_main.c
> >> index 9619b9e..c517194 100644
> >> --- a/drivers/net/ethernet/freescale/fec_main.c
> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
> >> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private
> >> *fep, unsigned ts,
> >>hwtstamps->hwtstamp = ns_to_ktime(ns);  }
> >>
> >> -static void
> >> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> >> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
> >> +  struct fec_enet_priv_tx_q *txq)
> >>  {
> >> -  struct  fec_enet_private *fep;
> >>struct bufdesc *bdp;
> >>unsigned short status;
> >>struct  sk_buff *skb;
> >> -  struct fec_enet_priv_tx_q *txq;
> >>struct netdev_queue *nq;
> >>int index = 0;
> >>int entries_free;
> >>
> >> -  fep = netdev_priv(ndev);
> >> -
> >> -  queue_id = FEC_ENET_GET_QUQUE(queue_id);
> >> -
> >> -  txq = fep->tx_queue[queue_id];
> >>/* get next bdp of dirty_tx */
> >> -  nq = netdev_get_tx_queue(ndev, queue_id);
> >> +  nq = netdev_get_tx_queue(ndev, txq->bd.qid);
> >>bdp = txq->dirty_tx;
> >>
> >>/* get next bdp of dirty_tx */
> >> @@ -1268,11 +1261,13 @@ static void
> >>  fec_enet_tx(struct net_device *ndev)  {
> >>struct fec_enet_private *fep = netdev_priv(ndev);
> >> +  struct fec_enet_priv_tx_q *txq;
> >>u16 queue_id;
> >>/* First process class A queue, then Class B and Best Effort queue */
> >>for_each_set_bit(queue_id, >work_tx, FEC_ENET_MAX_TX_QS)
> {
> >>clear_bit(queue_id, >work_tx);
> >> -  fec_enet_tx_queue(ndev, queue_id);
> >> +  txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> >> +  fec_txq(ndev, fep, txq);
> >>}
> >>return;
> >>  }
> >> --
> >> 2.5.0
> >
> > The patch should merge with patch#1.
> >
> 
> 
> Why ?  That would only hide the change in patch #1.

Hi Troy Kisky,

Sorry, I mean patch#2 net: fec: pass rxq to fec_enet_rx_queue instead of 
queue_id.  It is not necessary to separate them.

Regards,
Andy

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Troy Kisky]

On 3/2/2016 8:16 AM, Fugang Duan wrote:
> From: Troy Kisky   Sent: Thursday, February 
> 25, 2016 8:37 AM
>> To: netdev@vger.kernel.org; da...@davemloft.net; b38...@freescale.com
>> Cc: fabio.este...@freescale.com; l.st...@pengutronix.de; and...@lunn.ch;
>> trem...@gmail.com; li...@arm.linux.org.uk; linux-arm-
>> ker...@lists.infradead.org; l...@boundarydevices.com; shawn...@kernel.org;
>> johan...@sipsolutions.net; stillcompil...@gmail.com;
>> sergei.shtyl...@cogentembedded.com; a...@arndb.de; Troy Kisky
>> 
>> Subject: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue
>> instead of queue_id
>>
>> queue_id is the qid member of struct bufdesc_prop.
>>
>> Signed-off-by: Troy Kisky 
>> ---
>>  drivers/net/ethernet/freescale/fec_main.c | 17 ++---
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 9619b9e..c517194 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private *fep,
>> unsigned ts,
>>  hwtstamps->hwtstamp = ns_to_ktime(ns);  }
>>
>> -static void
>> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
>> +struct fec_enet_priv_tx_q *txq)
>>  {
>> -struct  fec_enet_private *fep;
>>  struct bufdesc *bdp;
>>  unsigned short status;
>>  struct  sk_buff *skb;
>> -struct fec_enet_priv_tx_q *txq;
>>  struct netdev_queue *nq;
>>  int index = 0;
>>  int entries_free;
>>
>> -fep = netdev_priv(ndev);
>> -
>> -queue_id = FEC_ENET_GET_QUQUE(queue_id);
>> -
>> -txq = fep->tx_queue[queue_id];
>>  /* get next bdp of dirty_tx */
>> -nq = netdev_get_tx_queue(ndev, queue_id);
>> +nq = netdev_get_tx_queue(ndev, txq->bd.qid);
>>  bdp = txq->dirty_tx;
>>
>>  /* get next bdp of dirty_tx */
>> @@ -1268,11 +1261,13 @@ static void
>>  fec_enet_tx(struct net_device *ndev)
>>  {
>>  struct fec_enet_private *fep = netdev_priv(ndev);
>> +struct fec_enet_priv_tx_q *txq;
>>  u16 queue_id;
>>  /* First process class A queue, then Class B and Best Effort queue */
>>  for_each_set_bit(queue_id, >work_tx, FEC_ENET_MAX_TX_QS)
>> {
>>  clear_bit(queue_id, >work_tx);
>> -fec_enet_tx_queue(ndev, queue_id);
>> +txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
>> +fec_txq(ndev, fep, txq);
>>  }
>>  return;
>>  }
>> --
>> 2.5.0
> 
> The patch should merge with patch#1.
> 


Why ?  That would only hide the change in patch #1.

RE: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Fugang Duan]

From: Troy Kisky   Sent: Thursday, February 25, 
2016 8:37 AM
> To: netdev@vger.kernel.org; da...@davemloft.net; b38...@freescale.com
> Cc: fabio.este...@freescale.com; l.st...@pengutronix.de; and...@lunn.ch;
> trem...@gmail.com; li...@arm.linux.org.uk; linux-arm-
> ker...@lists.infradead.org; l...@boundarydevices.com; shawn...@kernel.org;
> johan...@sipsolutions.net; stillcompil...@gmail.com;
> sergei.shtyl...@cogentembedded.com; a...@arndb.de; Troy Kisky
> 
> Subject: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue
> instead of queue_id
> 
> queue_id is the qid member of struct bufdesc_prop.
> 
> Signed-off-by: Troy Kisky 
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 9619b9e..c517194 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private *fep,
> unsigned ts,
>   hwtstamps->hwtstamp = ns_to_ktime(ns);  }
> 
> -static void
> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
> + struct fec_enet_priv_tx_q *txq)
>  {
> - struct  fec_enet_private *fep;
>   struct bufdesc *bdp;
>   unsigned short status;
>   struct  sk_buff *skb;
> - struct fec_enet_priv_tx_q *txq;
>   struct netdev_queue *nq;
>   int index = 0;
>   int entries_free;
> 
> - fep = netdev_priv(ndev);
> -
> - queue_id = FEC_ENET_GET_QUQUE(queue_id);
> -
> - txq = fep->tx_queue[queue_id];
>   /* get next bdp of dirty_tx */
> - nq = netdev_get_tx_queue(ndev, queue_id);
> + nq = netdev_get_tx_queue(ndev, txq->bd.qid);
>   bdp = txq->dirty_tx;
> 
>   /* get next bdp of dirty_tx */
> @@ -1268,11 +1261,13 @@ static void
>  fec_enet_tx(struct net_device *ndev)
>  {
>   struct fec_enet_private *fep = netdev_priv(ndev);
> + struct fec_enet_priv_tx_q *txq;
>   u16 queue_id;
>   /* First process class A queue, then Class B and Best Effort queue */
>   for_each_set_bit(queue_id, >work_tx, FEC_ENET_MAX_TX_QS)
> {
>   clear_bit(queue_id, >work_tx);
> - fec_enet_tx_queue(ndev, queue_id);
> + txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> + fec_txq(ndev, fep, txq);
>   }
>   return;
>  }
> --
> 2.5.0

The patch should merge with patch#1.

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Troy Kisky]

On 3/1/2016 3:26 PM, Zhi Li wrote:
> On Tue, Mar 1, 2016 at 3:51 PM, Troy Kisky
>  wrote:
>> True, but fec_txq/fec_rxq is called in a loop.
> 
> netdev_priv(ndev) is that pointer move.
> dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN)
> 
> Modem compiler can handle it greatly.
> 
> You can't get any valuable performance gain by that.
> 
> The main time of CPU is call dma_map_xxx.
> 
> best regards
> Frank Li
> 

Ok, I'll revert that part.

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Zhi Li]

On Tue, Mar 1, 2016 at 3:51 PM, Troy Kisky
 wrote:
> True, but fec_txq/fec_rxq is called in a loop.

netdev_priv(ndev) is that pointer move.
dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN)

Modem compiler can handle it greatly.

You can't get any valuable performance gain by that.

The main time of CPU is call dma_map_xxx.

best regards
Frank Li

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Troy Kisky]

On 3/1/2016 2:06 PM, Zhi Li wrote:
> On Wed, Feb 24, 2016 at 6:36 PM, Troy Kisky
>  wrote:
>> queue_id is the qid member of struct bufdesc_prop.
>>
>> Signed-off-by: Troy Kisky 
>> ---
>>  drivers/net/ethernet/freescale/fec_main.c | 17 ++---
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c 
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 9619b9e..c517194 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private *fep, 
>> unsigned ts,
>> hwtstamps->hwtstamp = ns_to_ktime(ns);
>>  }
>>
>> -static void
>> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
>> +   struct fec_enet_priv_tx_q *txq)
> 
> you can get fep from ndev.
> 

True, but fec_txq/fec_rxq is called in a loop. Why not pass it, rather than
look it up again?

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Zhi Li]

On Wed, Feb 24, 2016 at 6:36 PM, Troy Kisky
 wrote:
> queue_id is the qid member of struct bufdesc_prop.
>
> Signed-off-by: Troy Kisky 
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c 
> b/drivers/net/ethernet/freescale/fec_main.c
> index 9619b9e..c517194 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private *fep, 
> unsigned ts,
> hwtstamps->hwtstamp = ns_to_ktime(ns);
>  }
>
> -static void
> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
> +   struct fec_enet_priv_tx_q *txq)

you can get fep from ndev.

best regards
Frank Li

>  {
> -   struct  fec_enet_private *fep;
> struct bufdesc *bdp;
> unsigned short status;
> struct  sk_buff *skb;
> -   struct fec_enet_priv_tx_q *txq;
> struct netdev_queue *nq;
> int index = 0;
> int entries_free;
>
> -   fep = netdev_priv(ndev);
> -
> -   queue_id = FEC_ENET_GET_QUQUE(queue_id);
> -
> -   txq = fep->tx_queue[queue_id];
> /* get next bdp of dirty_tx */
> -   nq = netdev_get_tx_queue(ndev, queue_id);
> +   nq = netdev_get_tx_queue(ndev, txq->bd.qid);
> bdp = txq->dirty_tx;
>
> /* get next bdp of dirty_tx */
> @@ -1268,11 +1261,13 @@ static void
>  fec_enet_tx(struct net_device *ndev)
>  {
> struct fec_enet_private *fep = netdev_priv(ndev);
> +   struct fec_enet_priv_tx_q *txq;
> u16 queue_id;
> /* First process class A queue, then Class B and Best Effort queue */
> for_each_set_bit(queue_id, >work_tx, FEC_ENET_MAX_TX_QS) {
> clear_bit(queue_id, >work_tx);
> -   fec_enet_tx_queue(ndev, queue_id);
> +   txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> +   fec_txq(ndev, fep, txq);
> }
> return;
>  }
> --
> 2.5.0
>