Commit 0e00c05f authored by David S. Miller's avatar David S. Miller

Merge branch 'napi_gro_receive-caller-return-value-cleanups'

Jason A. Donenfeld says:

====================
napi_gro_receive caller return value cleanups

In 6570bc79 ("net: core: use listified Rx for GRO_NORMAL in
napi_gro_receive()"), the GRO_NORMAL case stopped calling
netif_receive_skb_internal, checking its return value, and returning
GRO_DROP in case it failed. Instead, it calls into
netif_receive_skb_list_internal (after a bit of indirection), which
doesn't return any error. Therefore, napi_gro_receive will never return
GRO_DROP, making handling GRO_DROP dead code.

I emailed the author of 6570bc79 on netdev [1] to see if this change
was intentional, but the dlink.ru email address has been disconnected,
and looking a bit further myself, it seems somewhat infeasible to start
propagating return values backwards from the internal machinations of
netif_receive_skb_list_internal.

Taking a look at all the callers of napi_gro_receive, it appears that
three are checking the return value for the purpose of comparing it to
the now never-happening GRO_DROP, and one just casts it to (void), a
likely historical leftover. Every other of the 120 callers does not
bother checking the return value.

And it seems like these remaining 116 callers are doing the right thing:
after calling napi_gro_receive, the packet is now in the hands of the
upper layers of the newtworking, and the device driver itself has no
business now making decisions based on what the upper layers choose to
do. Incrementing stats counters on GRO_DROP seems like a mistake, made
by these three drivers, but not by the remaining 117.

It would seem, therefore, that after rectifying these four callers of
napi_gro_receive, that I should go ahead and just remove returning the
value from napi_gro_receive all together. However, napi_gro_receive has
a function event tracer, and being able to introspect into the
networking stack to see how often napi_gro_receive is returning whatever
interesting GRO status (aside from _DROP) remains an interesting
data point worth keeping for debugging.

So, this series simply gets rid of the return value checking for the
four useless places where that check never evaluates to anything
meaningful.

[1] https://lore.kernel.org/netdev/20200624210606.GA1362687@zx2c4.com/
====================
Acked-by: default avatarEdward Cree <ecree@solarflare.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parents b18e9834 045790b7
...@@ -699,7 +699,7 @@ static void hns_nic_rx_up_pro(struct hns_nic_ring_data *ring_data, ...@@ -699,7 +699,7 @@ static void hns_nic_rx_up_pro(struct hns_nic_ring_data *ring_data,
struct net_device *ndev = ring_data->napi.dev; struct net_device *ndev = ring_data->napi.dev;
skb->protocol = eth_type_trans(skb, ndev); skb->protocol = eth_type_trans(skb, ndev);
(void)napi_gro_receive(&ring_data->napi, skb); napi_gro_receive(&ring_data->napi, skb);
} }
static int hns_desc_unused(struct hnae_ring *ring) static int hns_desc_unused(struct hnae_ring *ring)
......
...@@ -1044,8 +1044,9 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) ...@@ -1044,8 +1044,9 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
skb->ip_summed = CHECKSUM_UNNECESSARY; skb->ip_summed = CHECKSUM_UNNECESSARY;
next: next:
if ((skb && napi_gro_receive(&priv->napi, skb) != GRO_DROP) || if (skb)
xdp_result) { napi_gro_receive(&priv->napi, skb);
if (skb || xdp_result) {
ndev->stats.rx_packets++; ndev->stats.rx_packets++;
ndev->stats.rx_bytes += xdp.data_end - xdp.data; ndev->stats.rx_bytes += xdp.data_end - xdp.data;
} }
......
...@@ -414,14 +414,8 @@ static void wg_packet_consume_data_done(struct wg_peer *peer, ...@@ -414,14 +414,8 @@ static void wg_packet_consume_data_done(struct wg_peer *peer,
if (unlikely(routed_peer != peer)) if (unlikely(routed_peer != peer))
goto dishonest_packet_peer; goto dishonest_packet_peer;
if (unlikely(napi_gro_receive(&peer->napi, skb) == GRO_DROP)) { napi_gro_receive(&peer->napi, skb);
++dev->stats.rx_dropped; update_rx_stats(peer, message_data_len(len_before_trim));
net_dbg_ratelimited("%s: Failed to give packet to userspace from peer %llu (%pISpfsc)\n",
dev->name, peer->internal_id,
&peer->endpoint.addr);
} else {
update_rx_stats(peer, message_data_len(len_before_trim));
}
return; return;
dishonest_packet_peer: dishonest_packet_peer:
......
...@@ -897,7 +897,6 @@ static void wil_rx_handle_eapol(struct wil6210_vif *vif, struct sk_buff *skb) ...@@ -897,7 +897,6 @@ static void wil_rx_handle_eapol(struct wil6210_vif *vif, struct sk_buff *skb)
void wil_netif_rx(struct sk_buff *skb, struct net_device *ndev, int cid, void wil_netif_rx(struct sk_buff *skb, struct net_device *ndev, int cid,
struct wil_net_stats *stats, bool gro) struct wil_net_stats *stats, bool gro)
{ {
gro_result_t rc = GRO_NORMAL;
struct wil6210_vif *vif = ndev_to_vif(ndev); struct wil6210_vif *vif = ndev_to_vif(ndev);
struct wil6210_priv *wil = ndev_to_wil(ndev); struct wil6210_priv *wil = ndev_to_wil(ndev);
struct wireless_dev *wdev = vif_to_wdev(vif); struct wireless_dev *wdev = vif_to_wdev(vif);
...@@ -908,22 +907,16 @@ void wil_netif_rx(struct sk_buff *skb, struct net_device *ndev, int cid, ...@@ -908,22 +907,16 @@ void wil_netif_rx(struct sk_buff *skb, struct net_device *ndev, int cid,
*/ */
int mcast = is_multicast_ether_addr(da); int mcast = is_multicast_ether_addr(da);
struct sk_buff *xmit_skb = NULL; struct sk_buff *xmit_skb = NULL;
static const char * const gro_res_str[] = {
[GRO_MERGED] = "GRO_MERGED",
[GRO_MERGED_FREE] = "GRO_MERGED_FREE",
[GRO_HELD] = "GRO_HELD",
[GRO_NORMAL] = "GRO_NORMAL",
[GRO_DROP] = "GRO_DROP",
[GRO_CONSUMED] = "GRO_CONSUMED",
};
if (wdev->iftype == NL80211_IFTYPE_STATION) { if (wdev->iftype == NL80211_IFTYPE_STATION) {
sa = wil_skb_get_sa(skb); sa = wil_skb_get_sa(skb);
if (mcast && ether_addr_equal(sa, ndev->dev_addr)) { if (mcast && ether_addr_equal(sa, ndev->dev_addr)) {
/* mcast packet looped back to us */ /* mcast packet looped back to us */
rc = GRO_DROP;
dev_kfree_skb(skb); dev_kfree_skb(skb);
goto stats; ndev->stats.rx_dropped++;
stats->rx_dropped++;
wil_dbg_txrx(wil, "Rx drop %d bytes\n", len);
return;
} }
} else if (wdev->iftype == NL80211_IFTYPE_AP && !vif->ap_isolate) { } else if (wdev->iftype == NL80211_IFTYPE_AP && !vif->ap_isolate) {
if (mcast) { if (mcast) {
...@@ -967,26 +960,16 @@ void wil_netif_rx(struct sk_buff *skb, struct net_device *ndev, int cid, ...@@ -967,26 +960,16 @@ void wil_netif_rx(struct sk_buff *skb, struct net_device *ndev, int cid,
wil_rx_handle_eapol(vif, skb); wil_rx_handle_eapol(vif, skb);
if (gro) if (gro)
rc = napi_gro_receive(&wil->napi_rx, skb); napi_gro_receive(&wil->napi_rx, skb);
else else
netif_rx_ni(skb); netif_rx_ni(skb);
wil_dbg_txrx(wil, "Rx complete %d bytes => %s\n",
len, gro_res_str[rc]);
}
stats:
/* statistics. rc set to GRO_NORMAL for AP bridging */
if (unlikely(rc == GRO_DROP)) {
ndev->stats.rx_dropped++;
stats->rx_dropped++;
wil_dbg_txrx(wil, "Rx drop %d bytes\n", len);
} else {
ndev->stats.rx_packets++;
stats->rx_packets++;
ndev->stats.rx_bytes += len;
stats->rx_bytes += len;
if (mcast)
ndev->stats.multicast++;
} }
ndev->stats.rx_packets++;
stats->rx_packets++;
ndev->stats.rx_bytes += len;
stats->rx_bytes += len;
if (mcast)
ndev->stats.multicast++;
} }
void wil_netif_rx_any(struct sk_buff *skb, struct net_device *ndev) void wil_netif_rx_any(struct sk_buff *skb, struct net_device *ndev)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment