[net-next,v2,2/4] net: lan966x: Split function lan966x_fdma_rx_get_frame
Commit Message
The function lan966x_fdma_rx_get_frame was unmapping the frame from
device and check also if the frame was received on a valid port. And
only after that it tried to generate the skb.
Move this check in a different function, in preparation for xdp
support. Such that xdp to be added here and the
lan966x_fdma_rx_get_frame to be used only when giving the skb to upper
layers.
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
.../ethernet/microchip/lan966x/lan966x_fdma.c | 85 +++++++++++++------
.../ethernet/microchip/lan966x/lan966x_main.h | 9 ++
2 files changed, 69 insertions(+), 25 deletions(-)
Comments
From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: Sun, 6 Nov 2022 22:11:52 +0100
> The function lan966x_fdma_rx_get_frame was unmapping the frame from
> device and check also if the frame was received on a valid port. And
> only after that it tried to generate the skb.
> Move this check in a different function, in preparation for xdp
> support. Such that xdp to be added here and the
> lan966x_fdma_rx_get_frame to be used only when giving the skb to upper
> layers.
>
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
> .../ethernet/microchip/lan966x/lan966x_fdma.c | 85 +++++++++++++------
> .../ethernet/microchip/lan966x/lan966x_main.h | 9 ++
> 2 files changed, 69 insertions(+), 25 deletions(-)
[...]
> -static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
> +static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
> {
> struct lan966x *lan966x = rx->lan966x;
> - u64 src_port, timestamp;
> struct lan966x_db *db;
> - struct sk_buff *skb;
> struct page *page;
>
> - /* Get the received frame and unmap it */
> db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
> page = rx->page[rx->dcb_index][rx->db_index];
> + if (unlikely(!page))
> + return FDMA_ERROR;
>
> dma_sync_single_for_cpu(lan966x->dev, (dma_addr_t)db->dataptr,
> FDMA_DCB_STATUS_BLOCKL(db->status),
> DMA_FROM_DEVICE);
>
> + dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
> + PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
> + DMA_ATTR_SKIP_CPU_SYNC);
> +
> + lan966x_ifh_get_src_port(page_address(page), src_port);
> + if (WARN_ON(*src_port >= lan966x->num_phys_ports))
> + return FDMA_ERROR;
> +
> + return FDMA_PASS;
How about making this function return s64, which would be "src_port
or negative error", and dropping the second argument @src_port (the
example of calling it below)?
> +}
> +
> +static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
> + u64 src_port)
> +{
[...]
> - skb = lan966x_fdma_rx_get_frame(rx);
> + counter++;
>
> - rx->page[rx->dcb_index][rx->db_index] = NULL;
> - rx->dcb_index++;
> - rx->dcb_index &= FDMA_DCB_MAX - 1;
> + switch (lan966x_fdma_rx_check_frame(rx, &src_port)) {
> + case FDMA_PASS:
> + break;
> + case FDMA_ERROR:
> + lan966x_fdma_rx_free_page(rx);
> + lan966x_fdma_rx_advance_dcb(rx);
> + goto allocate_new;
> + }
So, here you could do (if you want to keep the current flow)::
src_port = lan966x_fdma_rx_check_frame(rx);
switch (src_port) {
case 0 .. S64_MAX: // for example
break;
case FDMA_ERROR: // must be < 0
lan_966x_fdma_rx_free_page(rx);
...
}
But given that the error path is very unlikely and cold, I would
prefer if-else over switch case:
src_port = lan966x_fdma_rx_check_frame(rx);
if (unlikely(src_port < 0)) {
lan_966x_fdma_rx_free_page(rx);
...
goto allocate_new;
}
>
> + skb = lan966x_fdma_rx_get_frame(rx, src_port);
> + lan966x_fdma_rx_advance_dcb(rx);
> if (!skb)
> - break;
> + goto allocate_new;
>
> napi_gro_receive(&lan966x->napi, skb);
> - counter++;
> }
>
> +allocate_new:
> /* Allocate new pages and map them */
> while (dcb_reload != rx->dcb_index) {
> db = &rx->dcbs[dcb_reload].db[rx->db_index];
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index 4ec33999e4df6..464fb5e4a8ff6 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -100,6 +100,15 @@ enum macaccess_entry_type {
> ENTRYTYPE_MACV6,
> };
>
> +/* FDMA return action codes for checking if the frame is valid
> + * FDMA_PASS, frame is valid and can be used
> + * FDMA_ERROR, something went wrong, stop getting more frames
> + */
> +enum lan966x_fdma_action {
> + FDMA_PASS = 0,
> + FDMA_ERROR,
> +};
> +
> struct lan966x_port;
>
> struct lan966x_db {
> --
> 2.38.0
Thanks,
Olek
The 11/07/2022 17:06, Alexander Lobakin wrote:
Hi Olek,
>
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Date: Sun, 6 Nov 2022 22:11:52 +0100
>
> > The function lan966x_fdma_rx_get_frame was unmapping the frame from
> > device and check also if the frame was received on a valid port. And
> > only after that it tried to generate the skb.
> > Move this check in a different function, in preparation for xdp
> > support. Such that xdp to be added here and the
> > lan966x_fdma_rx_get_frame to be used only when giving the skb to upper
> > layers.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> > .../ethernet/microchip/lan966x/lan966x_fdma.c | 85 +++++++++++++------
> > .../ethernet/microchip/lan966x/lan966x_main.h | 9 ++
> > 2 files changed, 69 insertions(+), 25 deletions(-)
>
> [...]
>
> > -static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
> > +static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
> > {
> > struct lan966x *lan966x = rx->lan966x;
> > - u64 src_port, timestamp;
> > struct lan966x_db *db;
> > - struct sk_buff *skb;
> > struct page *page;
> >
> > - /* Get the received frame and unmap it */
> > db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
> > page = rx->page[rx->dcb_index][rx->db_index];
> > + if (unlikely(!page))
> > + return FDMA_ERROR;
> >
> > dma_sync_single_for_cpu(lan966x->dev, (dma_addr_t)db->dataptr,
> > FDMA_DCB_STATUS_BLOCKL(db->status),
> > DMA_FROM_DEVICE);
> >
> > + dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
> > + PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
> > + DMA_ATTR_SKIP_CPU_SYNC);
> > +
> > + lan966x_ifh_get_src_port(page_address(page), src_port);
> > + if (WARN_ON(*src_port >= lan966x->num_phys_ports))
> > + return FDMA_ERROR;
> > +
> > + return FDMA_PASS;
>
> How about making this function return s64, which would be "src_port
> or negative error", and dropping the second argument @src_port (the
> example of calling it below)?
That was also my first thought.
But the thing is, I am also adding FDMA_DROP in the next patch of this
series(3/4). And I am planning to add also FDMA_TX and FDMA_REDIRECT in
a next patch series.
Should they(FDMA_DROP, FDMA_TX, FDMA_REDIRECT) also be some negative
numbers? And then have something like you proposed belowed:
---
src_port = lan966x_fdma_rx_check_frame(rx);
if (unlikely(src_port < 0)) {
switch(src_port) {
case FDMA_ERROR:
...
goto allocate_new
case FDMA_DROP:
...
continue;
case FDMA_TX:
case FDMA_REDIRECT:
}
}
---
>
> > +}
> > +
> > +static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
> > + u64 src_port)
> > +{
>
> [...]
>
> > - skb = lan966x_fdma_rx_get_frame(rx);
> > + counter++;
> >
> > - rx->page[rx->dcb_index][rx->db_index] = NULL;
> > - rx->dcb_index++;
> > - rx->dcb_index &= FDMA_DCB_MAX - 1;
> > + switch (lan966x_fdma_rx_check_frame(rx, &src_port)) {
> > + case FDMA_PASS:
> > + break;
> > + case FDMA_ERROR:
> > + lan966x_fdma_rx_free_page(rx);
> > + lan966x_fdma_rx_advance_dcb(rx);
> > + goto allocate_new;
> > + }
>
> So, here you could do (if you want to keep the current flow)::
>
> src_port = lan966x_fdma_rx_check_frame(rx);
> switch (src_port) {
> case 0 .. S64_MAX: // for example
> break;
> case FDMA_ERROR: // must be < 0
> lan_966x_fdma_rx_free_page(rx);
> ...
> }
>
> But given that the error path is very unlikely and cold, I would
> prefer if-else over switch case:
>
> src_port = lan966x_fdma_rx_check_frame(rx);
> if (unlikely(src_port < 0)) {
> lan_966x_fdma_rx_free_page(rx);
> ...
> goto allocate_new;
> }
>
> >
> > + skb = lan966x_fdma_rx_get_frame(rx, src_port);
> > + lan966x_fdma_rx_advance_dcb(rx);
> > if (!skb)
> > - break;
> > + goto allocate_new;
> >
> > napi_gro_receive(&lan966x->napi, skb);
> > - counter++;
> > }
> >
> > +allocate_new:
> > /* Allocate new pages and map them */
> > while (dcb_reload != rx->dcb_index) {
> > db = &rx->dcbs[dcb_reload].db[rx->db_index];
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > index 4ec33999e4df6..464fb5e4a8ff6 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > @@ -100,6 +100,15 @@ enum macaccess_entry_type {
> > ENTRYTYPE_MACV6,
> > };
> >
> > +/* FDMA return action codes for checking if the frame is valid
> > + * FDMA_PASS, frame is valid and can be used
> > + * FDMA_ERROR, something went wrong, stop getting more frames
> > + */
> > +enum lan966x_fdma_action {
> > + FDMA_PASS = 0,
> > + FDMA_ERROR,
> > +};
> > +
> > struct lan966x_port;
> >
> > struct lan966x_db {
> > --
> > 2.38.0
>
> Thanks,
> Olek
From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: Mon, 7 Nov 2022 22:24:15 +0100
> The 11/07/2022 17:06, Alexander Lobakin wrote:
>
> Hi Olek,
Hey,
>
> >
> > From: Horatiu Vultur <horatiu.vultur@microchip.com>
> > Date: Sun, 6 Nov 2022 22:11:52 +0100
> >
> > > The function lan966x_fdma_rx_get_frame was unmapping the frame from
> > > device and check also if the frame was received on a valid port. And
> > > only after that it tried to generate the skb.
> > > Move this check in a different function, in preparation for xdp
> > > support. Such that xdp to be added here and the
> > > lan966x_fdma_rx_get_frame to be used only when giving the skb to upper
> > > layers.
[...]
> > > + lan966x_ifh_get_src_port(page_address(page), src_port);
> > > + if (WARN_ON(*src_port >= lan966x->num_phys_ports))
> > > + return FDMA_ERROR;
> > > +
> > > + return FDMA_PASS;
> >
> > How about making this function return s64, which would be "src_port
> > or negative error", and dropping the second argument @src_port (the
> > example of calling it below)?
>
> That was also my first thought.
> But the thing is, I am also adding FDMA_DROP in the next patch of this
> series(3/4). And I am planning to add also FDMA_TX and FDMA_REDIRECT in
> a next patch series.
Yeah, I was reviewing the patches one by one and found out you're
adding more return values later :S
> Should they(FDMA_DROP, FDMA_TX, FDMA_REDIRECT) also be some negative
> numbers? And then have something like you proposed belowed:
> ---
> src_port = lan966x_fdma_rx_check_frame(rx);
> if (unlikely(src_port < 0)) {
>
> switch(src_port) {
> case FDMA_ERROR:
> ...
> goto allocate_new
> case FDMA_DROP:
> ...
> continue;
> case FDMA_TX:
> case FDMA_REDIRECT:
> }
It's okay to make them negative, but I wouldn't place them under
`unlikely`. It could be something like:
src_port = lan966x_fdma_rx_check_frame(rx);
if (unlikely(src_port == FDMA_ERROR))
goto allocate_new;
switch (src_port) {
case 0 ... S64_MAX:
// do PASS;
break;
case FDMA_TX:
// do TX;
break;
case FDMA_REDIRECT:
// and so on
}
where
enum {
FDMA_ERROR = -1, // only this one is "unlikely"
FDMA_TX = -2,
...
};
It's all just personal taste, so up to you :) Making
rx_check_frame() writing src_port to a pointer is fine as well.
> }
> ---
>
> >
> > > +}
> > > +
> > > +static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
> > > + u64 src_port)
> > > +{
[...]
> > > --
> > > 2.38.0
> >
> > Thanks,
> > Olek
>
> --
> /Horatiu
Thanks,
Olek
@@ -54,6 +54,17 @@ static void lan966x_fdma_rx_free_pages(struct lan966x_rx *rx)
}
}
+static void lan966x_fdma_rx_free_page(struct lan966x_rx *rx)
+{
+ struct page *page;
+
+ page = rx->page[rx->dcb_index][rx->db_index];
+ if (unlikely(!page))
+ return;
+
+ __free_pages(page, rx->page_order);
+}
+
static void lan966x_fdma_rx_add_dcb(struct lan966x_rx *rx,
struct lan966x_rx_dcb *dcb,
u64 nextptr)
@@ -116,6 +127,12 @@ static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
return 0;
}
+static void lan966x_fdma_rx_advance_dcb(struct lan966x_rx *rx)
+{
+ rx->dcb_index++;
+ rx->dcb_index &= FDMA_DCB_MAX - 1;
+}
+
static void lan966x_fdma_rx_free(struct lan966x_rx *rx)
{
struct lan966x *lan966x = rx->lan966x;
@@ -403,38 +420,53 @@ static bool lan966x_fdma_rx_more_frames(struct lan966x_rx *rx)
return true;
}
-static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
+static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
{
struct lan966x *lan966x = rx->lan966x;
- u64 src_port, timestamp;
struct lan966x_db *db;
- struct sk_buff *skb;
struct page *page;
- /* Get the received frame and unmap it */
db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
page = rx->page[rx->dcb_index][rx->db_index];
+ if (unlikely(!page))
+ return FDMA_ERROR;
dma_sync_single_for_cpu(lan966x->dev, (dma_addr_t)db->dataptr,
FDMA_DCB_STATUS_BLOCKL(db->status),
DMA_FROM_DEVICE);
+ dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
+ PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
+ DMA_ATTR_SKIP_CPU_SYNC);
+
+ lan966x_ifh_get_src_port(page_address(page), src_port);
+ if (WARN_ON(*src_port >= lan966x->num_phys_ports))
+ return FDMA_ERROR;
+
+ return FDMA_PASS;
+}
+
+static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
+ u64 src_port)
+{
+ struct lan966x *lan966x = rx->lan966x;
+ struct lan966x_db *db;
+ struct sk_buff *skb;
+ struct page *page;
+ u64 timestamp;
+
+ /* Get the received frame and unmap it */
+ db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
+ page = rx->page[rx->dcb_index][rx->db_index];
+
skb = build_skb(page_address(page), PAGE_SIZE << rx->page_order);
if (unlikely(!skb))
- goto unmap_page;
+ goto free_page;
skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));
- lan966x_ifh_get_src_port(skb->data, &src_port);
lan966x_ifh_get_timestamp(skb->data, ×tamp);
- if (WARN_ON(src_port >= lan966x->num_phys_ports))
- goto free_skb;
-
- dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
- PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
- DMA_ATTR_SKIP_CPU_SYNC);
-
skb->dev = lan966x->ports[src_port]->dev;
skb_pull(skb, IFH_LEN_BYTES);
@@ -457,12 +489,7 @@ static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
return skb;
-free_skb:
- kfree_skb(skb);
-unmap_page:
- dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
- PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
- DMA_ATTR_SKIP_CPU_SYNC);
+free_page:
__free_pages(page, rx->page_order);
return NULL;
@@ -478,6 +505,7 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
struct sk_buff *skb;
struct page *page;
int counter = 0;
+ u64 src_port;
u64 nextptr;
lan966x_fdma_tx_clear_buf(lan966x, weight);
@@ -487,19 +515,26 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
if (!lan966x_fdma_rx_more_frames(rx))
break;
- skb = lan966x_fdma_rx_get_frame(rx);
+ counter++;
- rx->page[rx->dcb_index][rx->db_index] = NULL;
- rx->dcb_index++;
- rx->dcb_index &= FDMA_DCB_MAX - 1;
+ switch (lan966x_fdma_rx_check_frame(rx, &src_port)) {
+ case FDMA_PASS:
+ break;
+ case FDMA_ERROR:
+ lan966x_fdma_rx_free_page(rx);
+ lan966x_fdma_rx_advance_dcb(rx);
+ goto allocate_new;
+ }
+ skb = lan966x_fdma_rx_get_frame(rx, src_port);
+ lan966x_fdma_rx_advance_dcb(rx);
if (!skb)
- break;
+ goto allocate_new;
napi_gro_receive(&lan966x->napi, skb);
- counter++;
}
+allocate_new:
/* Allocate new pages and map them */
while (dcb_reload != rx->dcb_index) {
db = &rx->dcbs[dcb_reload].db[rx->db_index];
@@ -100,6 +100,15 @@ enum macaccess_entry_type {
ENTRYTYPE_MACV6,
};
+/* FDMA return action codes for checking if the frame is valid
+ * FDMA_PASS, frame is valid and can be used
+ * FDMA_ERROR, something went wrong, stop getting more frames
+ */
+enum lan966x_fdma_action {
+ FDMA_PASS = 0,
+ FDMA_ERROR,
+};
+
struct lan966x_port;
struct lan966x_db {