| Message ID | 20240925162048.16208-3-jdamato@fastly.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tg3: Link IRQs, NAPIs, and queues | expand |
On Wed, Sep 25, 2024 at 04:20:48PM +0000, Joe Damato wrote: > Link queues to NAPIs using the netdev-genl API so this information is > queryable. > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump queue-get --json='{"ifindex": 2}' > > [{'id': 0, 'ifindex': 2, 'type': 'rx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}] > > Signed-off-by: Joe Damato <jdamato@fastly.com> > --- > drivers/net/ethernet/broadcom/tg3.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c > index ddf0bb65c929..f78d7e8c40b2 100644 > --- a/drivers/net/ethernet/broadcom/tg3.c > +++ b/drivers/net/ethernet/broadcom/tg3.c > @@ -7395,18 +7395,34 @@ static int tg3_poll(struct napi_struct *napi, int budget) > > static void tg3_napi_disable(struct tg3 *tp) > { > + struct tg3_napi *tnapi; > int i; > > - for (i = tp->irq_cnt - 1; i >= 0; i--) > - napi_disable(&tp->napi[i].napi); > + ASSERT_RTNL(); > + for (i = tp->irq_cnt - 1; i >= 0; i--) { > + tnapi = &tp->napi[i]; > + if (tnapi->tx_buffers) > + netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, NULL); It looks like the ASSERT_RTNL is unnecessary; netif_queue_set_napi will call it internally, so I'll remove it before sending this to the list (barring any other feedback). > static void tg3_napi_enable(struct tg3 *tp) > { > + struct tg3_napi *tnapi; > int i; > > - for (i = 0; i < tp->irq_cnt; i++) > - napi_enable(&tp->napi[i].napi); > + ASSERT_RTNL(); > + for (i = 0; i < tp->irq_cnt; i++) { > + tnapi = &tp->napi[i]; > + napi_enable(&tnapi->napi); > + if (tnapi->tx_buffers) > + netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, &tnapi->napi); Same as above.
On Fri, Sep 27, 2024 at 4:47 AM Joe Damato <jdamato@fastly.com> wrote: > > On Wed, Sep 25, 2024 at 04:20:48PM +0000, Joe Damato wrote: > > Link queues to NAPIs using the netdev-genl API so this information is > > queryable. > > > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > > --dump queue-get --json='{"ifindex": 2}' > > > > [{'id': 0, 'ifindex': 2, 'type': 'rx'}, > > {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'}, > > {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'}, > > {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'}, > > {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}] > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > --- > > drivers/net/ethernet/broadcom/tg3.c | 24 ++++++++++++++++++++---- > > 1 file changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c > > index ddf0bb65c929..f78d7e8c40b2 100644 > > --- a/drivers/net/ethernet/broadcom/tg3.c > > +++ b/drivers/net/ethernet/broadcom/tg3.c > > @@ -7395,18 +7395,34 @@ static int tg3_poll(struct napi_struct *napi, int budget) > > > > static void tg3_napi_disable(struct tg3 *tp) > > { > > + struct tg3_napi *tnapi; > > int i; > > > > - for (i = tp->irq_cnt - 1; i >= 0; i--) > > - napi_disable(&tp->napi[i].napi); > > + ASSERT_RTNL(); > > + for (i = tp->irq_cnt - 1; i >= 0; i--) { > > + tnapi = &tp->napi[i]; > > + if (tnapi->tx_buffers) > > + netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, NULL); > > It looks like the ASSERT_RTNL is unnecessary; netif_queue_set_napi > will call it internally, so I'll remove it before sending this to > the list (barring any other feedback). Thanks LGTM. You can use Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> > > > static void tg3_napi_enable(struct tg3 *tp) > > { > > + struct tg3_napi *tnapi; > > int i; > > > > - for (i = 0; i < tp->irq_cnt; i++) > > - napi_enable(&tp->napi[i].napi); > > + ASSERT_RTNL(); > > + for (i = 0; i < tp->irq_cnt; i++) { > > + tnapi = &tp->napi[i]; > > + napi_enable(&tnapi->napi); > > + if (tnapi->tx_buffers) > > + netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, &tnapi->napi); > > Same as above.
On Fri, Sep 27, 2024 at 09:33:51AM +0530, Pavan Chebbi wrote: > On Fri, Sep 27, 2024 at 4:47 AM Joe Damato <jdamato@fastly.com> wrote: > > > > On Wed, Sep 25, 2024 at 04:20:48PM +0000, Joe Damato wrote: > > > Link queues to NAPIs using the netdev-genl API so this information is > > > queryable. > > > > > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > > > --dump queue-get --json='{"ifindex": 2}' > > > > > > [{'id': 0, 'ifindex': 2, 'type': 'rx'}, > > > {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'}, > > > {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'}, > > > {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'}, > > > {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}] > > > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > > --- > > > drivers/net/ethernet/broadcom/tg3.c | 24 ++++++++++++++++++++---- > > > 1 file changed, 20 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c > > > index ddf0bb65c929..f78d7e8c40b2 100644 > > > --- a/drivers/net/ethernet/broadcom/tg3.c > > > +++ b/drivers/net/ethernet/broadcom/tg3.c > > > @@ -7395,18 +7395,34 @@ static int tg3_poll(struct napi_struct *napi, int budget) > > > > > > static void tg3_napi_disable(struct tg3 *tp) > > > { > > > + struct tg3_napi *tnapi; > > > int i; > > > > > > - for (i = tp->irq_cnt - 1; i >= 0; i--) > > > - napi_disable(&tp->napi[i].napi); > > > + ASSERT_RTNL(); > > > + for (i = tp->irq_cnt - 1; i >= 0; i--) { > > > + tnapi = &tp->napi[i]; > > > + if (tnapi->tx_buffers) > > > + netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, NULL); > > > > It looks like the ASSERT_RTNL is unnecessary; netif_queue_set_napi > > will call it internally, so I'll remove it before sending this to > > the list (barring any other feedback). > > Thanks LGTM. You can use Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> Thanks, I've added your Reviewed-by for the official submission. I'll mention this in the cover letter when net-next is open but the only changes I've made to the patch I posted are: - Removal of ASSERT_RTNL (mentioned above, as it seems to be unnecessary) - Wrapped lines at 80 characters (cosmetic change only)
On Fri, Sep 27, 2024 at 09:33:51AM +0530, Pavan Chebbi wrote: > On Fri, Sep 27, 2024 at 4:47 AM Joe Damato <jdamato@fastly.com> wrote: > > > > On Wed, Sep 25, 2024 at 04:20:48PM +0000, Joe Damato wrote: > > > Link queues to NAPIs using the netdev-genl API so this information is > > > queryable. > > > > > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > > > --dump queue-get --json='{"ifindex": 2}' > > > > > > [{'id': 0, 'ifindex': 2, 'type': 'rx'}, > > > {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'}, > > > {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'}, > > > {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'}, > > > {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}] > > > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > > --- > > > drivers/net/ethernet/broadcom/tg3.c | 24 ++++++++++++++++++++---- > > > 1 file changed, 20 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c > > > index ddf0bb65c929..f78d7e8c40b2 100644 > > > --- a/drivers/net/ethernet/broadcom/tg3.c > > > +++ b/drivers/net/ethernet/broadcom/tg3.c > > > @@ -7395,18 +7395,34 @@ static int tg3_poll(struct napi_struct *napi, int budget) > > > > > > static void tg3_napi_disable(struct tg3 *tp) > > > { > > > + struct tg3_napi *tnapi; > > > int i; > > > > > > - for (i = tp->irq_cnt - 1; i >= 0; i--) > > > - napi_disable(&tp->napi[i].napi); > > > + ASSERT_RTNL(); > > > + for (i = tp->irq_cnt - 1; i >= 0; i--) { > > > + tnapi = &tp->napi[i]; > > > + if (tnapi->tx_buffers) > > > + netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, NULL); > > > > It looks like the ASSERT_RTNL is unnecessary; netif_queue_set_napi > > will call it internally, so I'll remove it before sending this to > > the list (barring any other feedback). > > Thanks LGTM. You can use Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> I noticed there's a misnumbering issue in the code. Note the output from the first patch: [{'id': 149, 'ifindex': 2, 'irq': 335}, {'id': 148, 'ifindex': 2, 'irq': 334}, {'id': 147, 'ifindex': 2, 'irq': 333}, {'id': 146, 'ifindex': 2, 'irq': 332}, {'id': 145, 'ifindex': 2, 'irq': 331}] Note the output in the commit message above: [{'id': 0, 'ifindex': 2, 'type': 'rx'}, {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'}, {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'}, {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'}, {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}] Note that id 0 type: 'rx' has no napi-id associated with it, and in the second block, NAPI ID 149 is nowhere to be found. This is happening because the code in the driver does this: for (i = 0; i < tp->irq_cnt; i++) { tnapi = &tp->napi[i]; napi_enable(&tnapi->napi); if (tnapi->tx_buffers) netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, &tnapi->napi); The code I added assumed that i is the txq or rxq index, but it's not - it's the index into the array of struct tg3_napi. Corrected, the code looks like something like this: int txq_idx = 0, rxq_idx = 0; [...] for (i = 0; i < tp->irq_cnt; i++) { tnapi = &tp->napi[i]; napi_enable(&tnapi->napi); if (tnapi->tx_buffers) { netif_queue_set_napi(tp->dev, txq_idx, NETDEV_QUEUE_TYPE_TX, &tnapi->napi); txq_idx++ } else if (tnapi->rx_rcb) { netif_queue_set_napi(tp->dev, rxq_idx, NETDEV_QUEUE_TYPE_RX, &tnapi->napi); rxq_idx++; [...] I tested that and the output looks correct to me. However, what to do about tg3_napi_disable ? Probably something like this (txq only for brevity): int txq_idx = tp->txq_cnt - 1; [...] for (i = tp->irq_cnt - 1; i >= 0; i--) { [...] if (tnapi->tx_buffers) { netif_queue_set_napi(tp->dev, txq_idx, NETDEV_QUEUE_TYPE_TX, NULL); txq_idx--; } [...] Does that seem correct to you? I wanted to ask before sending another revision, since I am not a tg3 expert. I will of course remove your Reviewed-by from this patch (but leave it on patch 1 which is unmodified) when I resend it.
On Thu, Oct 3, 2024 at 4:51 AM Joe Damato <jdamato@fastly.com> wrote: > > This is happening because the code in the driver does this: > > for (i = 0; i < tp->irq_cnt; i++) { > tnapi = &tp->napi[i]; > napi_enable(&tnapi->napi); > if (tnapi->tx_buffers) > netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, > &tnapi->napi); > > The code I added assumed that i is the txq or rxq index, but it's > not - it's the index into the array of struct tg3_napi. Yes, you are right.. > > Corrected, the code looks like something like this: > > int txq_idx = 0, rxq_idx = 0; > [...] > > for (i = 0; i < tp->irq_cnt; i++) { > tnapi = &tp->napi[i]; > napi_enable(&tnapi->napi); > if (tnapi->tx_buffers) { > netif_queue_set_napi(tp->dev, txq_idx, NETDEV_QUEUE_TYPE_TX, > &tnapi->napi); > txq_idx++ > } else if (tnapi->rx_rcb) { > netif_queue_set_napi(tp->dev, rxq_idx, NETDEV_QUEUE_TYPE_RX, > &tnapi->napi); > rxq_idx++; > [...] > > I tested that and the output looks correct to me. However, what to > do about tg3_napi_disable ? > > Probably something like this (txq only for brevity): > > int txq_idx = tp->txq_cnt - 1; > [...] > > for (i = tp->irq_cnt - 1; i >= 0; i--) { > [...] > if (tnapi->tx_buffers) { > netif_queue_set_napi(tp->dev, txq_idx, NETDEV_QUEUE_TYPE_TX, > NULL); > txq_idx--; > } > [...] > > Does that seem correct to you? I wanted to ask before sending > another revision, since I am not a tg3 expert. > The local counter variable for the ring ids might work because irqs are requested sequentially. Thinking out loud, a better way would be to save the tx/rx id inside their struct tg3_napi in the tg3_request_irq() function. And have a separate new function (I know you did something similar for v1 of irq-napi linking) to link queues and napi. I think it should work, and should help during de-linking also. Let me know what you think.
On Thu, Oct 03, 2024 at 09:56:40AM +0530, Pavan Chebbi wrote: > On Thu, Oct 3, 2024 at 4:51 AM Joe Damato <jdamato@fastly.com> wrote: > > > > > This is happening because the code in the driver does this: > > > > for (i = 0; i < tp->irq_cnt; i++) { > > tnapi = &tp->napi[i]; > > napi_enable(&tnapi->napi); > > if (tnapi->tx_buffers) > > netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, > > &tnapi->napi); > > > > The code I added assumed that i is the txq or rxq index, but it's > > not - it's the index into the array of struct tg3_napi. > > Yes, you are right.. > > > > Corrected, the code looks like something like this: > > > > int txq_idx = 0, rxq_idx = 0; > > [...] > > > > for (i = 0; i < tp->irq_cnt; i++) { > > tnapi = &tp->napi[i]; > > napi_enable(&tnapi->napi); > > if (tnapi->tx_buffers) { > > netif_queue_set_napi(tp->dev, txq_idx, NETDEV_QUEUE_TYPE_TX, > > &tnapi->napi); > > txq_idx++ > > } else if (tnapi->rx_rcb) { > > netif_queue_set_napi(tp->dev, rxq_idx, NETDEV_QUEUE_TYPE_RX, > > &tnapi->napi); > > rxq_idx++; > > [...] > > > > I tested that and the output looks correct to me. However, what to > > do about tg3_napi_disable ? > > > > Probably something like this (txq only for brevity): > > > > int txq_idx = tp->txq_cnt - 1; > > [...] > > > > for (i = tp->irq_cnt - 1; i >= 0; i--) { > > [...] > > if (tnapi->tx_buffers) { > > netif_queue_set_napi(tp->dev, txq_idx, NETDEV_QUEUE_TYPE_TX, > > NULL); > > txq_idx--; > > } > > [...] > > > > Does that seem correct to you? I wanted to ask before sending > > another revision, since I am not a tg3 expert. > > > > The local counter variable for the ring ids might work because irqs > are requested sequentially. Yea, my proposal relies on the sequential ordering. > Thinking out loud, a better way would be to save the tx/rx id inside > their struct tg3_napi in the tg3_request_irq() function. I think that could work, yes. I wasn't sure if you'd be open to such a change. It seems like in that case, though, we'd need to add some state somewhere. It's not super clear to me where the appropriate place for the state would be because tg3_request_irq is called in a couple places (like tg3_test_interrupt). Another option would be to modify tg3_enable_msix and modify: for (i = 0; i < tp->irq_max; i++) tp->napi[i].irq_vec = msix_ent[i].vector; But, all of that is still a bit invasive compared to the running rxq_idx txq_idx counters I proposed in my previous message. I am open to doing whatever you suggest/prefer, though, since it is your driver after all :) > And have a separate new function (I know you did something similar for > v1 of irq-napi linking) to link queues and napi. > I think it should work, and should help during de-linking also. Let me > know what you think. I think it's possible, it's just disruptive and it's not clear if it's worth it? Some other code path might break and it might be fine to just rely on the sequential indexing? Not sure. Let me know what you think; thanks for taking the time to review and respond.
> > The local counter variable for the ring ids might work because irqs > > are requested sequentially. > > Yea, my proposal relies on the sequential ordering. > > > Thinking out loud, a better way would be to save the tx/rx id inside > > their struct tg3_napi in the tg3_request_irq() function. > > I think that could work, yes. I wasn't sure if you'd be open to such > a change. > > It seems like in that case, though, we'd need to add some state > somewhere. > > It's not super clear to me where the appropriate place for the state > would be because tg3_request_irq is called in a couple places (like > tg3_test_interrupt). > > Another option would be to modify tg3_enable_msix and modify: > > for (i = 0; i < tp->irq_max; i++) > tp->napi[i].irq_vec = msix_ent[i].vector; Hi Joe, not in favor of this change. > > But, all of that is still a bit invasive compared to the running > rxq_idx txq_idx counters I proposed in my previous message. > > I am open to doing whatever you suggest/prefer, though, since it is > your driver after all :) > > > And have a separate new function (I know you did something similar for > > v1 of irq-napi linking) to link queues and napi. > > I think it should work, and should help during de-linking also. Let me > > know what you think. > > I think it's possible, it's just disruptive and it's not clear if > it's worth it? Some other code path might break and it might be fine > to just rely on the sequential indexing? Not sure. > I don't have strong opposition to your proposal of using local counters. Just that an alternate solution like what I suggested may look less arbitrary, imo. So if you want to use the local counters you may go ahead unless Michael has any other suggestions. > Let me know what you think; thanks for taking the time to review and > respond.
On Fri, Oct 04, 2024 at 09:03:58PM +0530, Pavan Chebbi wrote: [...] > > > Thinking out loud, a better way would be to save the tx/rx id inside > > > their struct tg3_napi in the tg3_request_irq() function. > > > > I think that could work, yes. I wasn't sure if you'd be open to such > > a change. > > > > It seems like in that case, though, we'd need to add some state > > somewhere. > > > > It's not super clear to me where the appropriate place for the state > > would be because tg3_request_irq is called in a couple places (like > > tg3_test_interrupt). > > > > Another option would be to modify tg3_enable_msix and modify: > > > > for (i = 0; i < tp->irq_max; i++) > > tp->napi[i].irq_vec = msix_ent[i].vector; > Hi Joe, not in favor of this change. OK [...] > > I think it's possible, it's just disruptive and it's not clear if > > it's worth it? Some other code path might break and it might be fine > > to just rely on the sequential indexing? Not sure. > > > I don't have strong opposition to your proposal of using local counters. > Just that an alternate solution like what I suggested may look less > arbitrary, imo. I don't see where the state would be added for tracking the current rxq_idx and txq_idx, though. And I don't necessarily agree that the counters are arbitrary? Unless tg3 is currently rx and tx index somewhere, then just assuming they are linear seems fine ? > So if you want to use the local counters you may go ahead unless > Michael has any other suggestions. I'll send another RFC and you can see what it looks like before deciding.
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index ddf0bb65c929..f78d7e8c40b2 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -7395,18 +7395,34 @@ static int tg3_poll(struct napi_struct *napi, int budget) static void tg3_napi_disable(struct tg3 *tp) { + struct tg3_napi *tnapi; int i; - for (i = tp->irq_cnt - 1; i >= 0; i--) - napi_disable(&tp->napi[i].napi); + ASSERT_RTNL(); + for (i = tp->irq_cnt - 1; i >= 0; i--) { + tnapi = &tp->napi[i]; + if (tnapi->tx_buffers) + netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, NULL); + else if (tnapi->rx_rcb) + netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_RX, NULL); + napi_disable(&tnapi->napi); + } } static void tg3_napi_enable(struct tg3 *tp) { + struct tg3_napi *tnapi; int i; - for (i = 0; i < tp->irq_cnt; i++) - napi_enable(&tp->napi[i].napi); + ASSERT_RTNL(); + for (i = 0; i < tp->irq_cnt; i++) { + tnapi = &tp->napi[i]; + napi_enable(&tnapi->napi); + if (tnapi->tx_buffers) + netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, &tnapi->napi); + else if (tnapi->rx_rcb) + netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_RX, &tnapi->napi); + } } static void tg3_napi_init(struct tg3 *tp)
Link queues to NAPIs using the netdev-genl API so this information is queryable. $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ --dump queue-get --json='{"ifindex": 2}' [{'id': 0, 'ifindex': 2, 'type': 'rx'}, {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'}, {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'}, {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'}, {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}] Signed-off-by: Joe Damato <jdamato@fastly.com> --- drivers/net/ethernet/broadcom/tg3.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)