[v3] batman-adv: Introduce forward packet creation helper
Commit Message
This patch abstracts the forward packet creation into the new function
batadv_forw_packet_alloc().
The queue counting and interface reference counters are now handled
internally within batadv_forw_packet_alloc() and its
batadv_forw_packet_free() counterpart. This should reduce the risk of
having reference/queue counting bugs again and should increase
code readibility.
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
Changelog v3:
* Fixed kernel doc for batadv_forw_packet_alloc()
* New title / some more adjustments to commit message
Changelog v2:
* Rebase to current master
* Fixed typo: "ressources" vs. "resources"
* Moved forw_packet->num_packets initialization into forw_packet_alloc(), too
* Adjusted commit message
net/batman-adv/bat_iv_ogm.c | 34 ++++-----------
net/batman-adv/send.c | 102 ++++++++++++++++++++++++++++++++------------
net/batman-adv/send.h | 6 +++
net/batman-adv/types.h | 2 +
4 files changed, 91 insertions(+), 53 deletions(-)
Comments
On Tuesday 14 June 2016 23:19:43 Linus Lüssing wrote:
[...]
> +struct batadv_forw_packet *
> +batadv_forw_packet_alloc(struct batadv_hard_iface *if_incoming,
> + struct batadv_hard_iface *if_outgoing,
> + atomic_t *queue_left,
> + struct batadv_priv *bat_priv)
> +{
> + struct batadv_forw_packet *forw_packet = NULL;
> +
> + if (queue_left && !batadv_atomic_dec_not_zero(queue_left)) {
> + if (queue_left == &bat_priv->bcast_queue_left)
> + batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> + "bcast queue full\n");
> + else if (queue_left == &bat_priv->batman_queue_left)
> + batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> + "batman queue full\n");
> + else
> + batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> + "a mysterious queue is full\n");
> + goto out;
> + }
Returning here directly is most likely easier to read. At least I had to
check twice if the gotos in this function are all correct.
And I would have written it slightly different. I don't want to say that your
way is worse or my version is better but just add a slightly different way
into the discussion
struct batadv_forw_packet *forw_packet;
const char *qname;
if (queue_left && !batadv_atomic_dec_not_zero(queue_left)) {
qname = "unknown";
if (queue_left == &bat_priv->bcast_queue_left)
qname = "bcast";
if (queue_left == &bat_priv->batman_queue_left)
qname = "batman";
batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "%s queue is full\n",
qname);
return NULL;
}
> + goto out;
> +
> +err:
> + if (queue_left)
> + atomic_inc(queue_left);
> +out:
> + return forw_packet;
> +}
Maybe we can return here directly forw_packet and in the error case
return NULL;
return forw_packet;
err_inc_queue_left:
if (queue_left)
atomic_inc(queue_left);
return NULL;
> @@ -478,20 +546,16 @@ int batadv_add_bcast_packet_to_list(struct batadv_priv *bat_priv,
> struct batadv_bcast_packet *bcast_packet;
> struct sk_buff *newskb;
>
> - if (!batadv_atomic_dec_not_zero(&bat_priv->bcast_queue_left)) {
> - batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> - "bcast packet queue full\n");
> - goto out;
> - }
> -
> primary_if = batadv_primary_if_get_selected(bat_priv);
> if (!primary_if)
> - goto out_and_inc;
> -
> - forw_packet = kmalloc(sizeof(*forw_packet), GFP_ATOMIC);
> + goto out;
I think you can return here directly
>
> + forw_packet = batadv_forw_packet_alloc(primary_if, NULL,
> + &bat_priv->bcast_queue_left,
> + bat_priv);
> + batadv_hardif_put(primary_if);
> if (!forw_packet)
> - goto out_and_inc;
> + goto out;
Same here
All the mentioned things are just nitpicking. So overall:
Reviewed-by: Sven Eckelmann <sven@narfation.org>
Please keep the Reviewed-by in case you you resent the patch and only
include changes which were mentioned in this mail.
Kind regards,
Sven
On Sun, Jun 19, 2016 at 01:00:18PM +0200, Sven Eckelmann wrote:
> And I would have written it slightly different. I don't want to say that your
> way is worse or my version is better but just add a slightly different way
> into the discussion
Looks good to me, I like all your suggestions for
forw_packet_alloc() :).
> > @@ -478,20 +546,16 @@ int batadv_add_bcast_packet_to_list(struct batadv_priv *bat_priv,
> > struct batadv_bcast_packet *bcast_packet;
> > struct sk_buff *newskb;
> >
> > - if (!batadv_atomic_dec_not_zero(&bat_priv->bcast_queue_left)) {
> > - batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> > - "bcast packet queue full\n");
> > - goto out;
> > - }
> > -
> > primary_if = batadv_primary_if_get_selected(bat_priv);
> > if (!primary_if)
> > - goto out_and_inc;
> > -
> > - forw_packet = kmalloc(sizeof(*forw_packet), GFP_ATOMIC);
> > + goto out;
>
> I think you can return here directly
>
> >
> > + forw_packet = batadv_forw_packet_alloc(primary_if, NULL,
> > + &bat_priv->bcast_queue_left,
> > + bat_priv);
> > + batadv_hardif_put(primary_if);
> > if (!forw_packet)
> > - goto out_and_inc;
> > + goto out;
>
> Same here
Regarding these two goto's, usually using a "return" directly instead of a
"goto out" is better, I agree. For these two cases I think I would
prefer keeping it that way, because it's not a simple return but a
return of a macro value. Having NETDEV_TX_OK and NETDEV_TX_BUSY
each only once in a function makes it easier to spot the good or
bad cases, I think?
Or maybe I could rename "out" and "packet_free" to "err" and
"err_packet_free" to make the error cases even more visible?
Regards, Linus
On Monday 20 June 2016 19:58:03 Linus Lüssing wrote:
[...]
> > > @@ -478,20 +546,16 @@ int batadv_add_bcast_packet_to_list(struct batadv_priv *bat_priv,
[...]
> Regarding these two goto's, usually using a "return" directly instead of a
> "goto out" is better, I agree. For these two cases I think I would
> prefer keeping it that way, because it's not a simple return but a
> return of a macro value. Having NETDEV_TX_OK and NETDEV_TX_BUSY
> each only once in a function makes it easier to spot the good or
> bad cases, I think?
>
> Or maybe I could rename "out" and "packet_free" to "err" and
> "err_packet_free" to make the error cases even more visible?
Hm, not sure if this is really required but I honestly like the
names "err" and "err_packet_free" right now :)
Kind regards,
Sven
@@ -685,19 +685,12 @@ static void batadv_iv_ogm_aggregate_new(const unsigned char *packet_buff,
struct batadv_forw_packet *forw_packet_aggr;
unsigned char *skb_buff;
unsigned int skb_size;
+ atomic_t *queue_left = own_packet ? NULL : &bat_priv->batman_queue_left;
- /* own packet should always be scheduled */
- if (!own_packet) {
- if (!batadv_atomic_dec_not_zero(&bat_priv->batman_queue_left)) {
- batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
- "batman packet queue full\n");
- return;
- }
- }
-
- forw_packet_aggr = kmalloc(sizeof(*forw_packet_aggr), GFP_ATOMIC);
+ forw_packet_aggr = batadv_forw_packet_alloc(if_incoming, if_outgoing,
+ queue_left, bat_priv);
if (!forw_packet_aggr)
- goto out_nomem;
+ return;
if (atomic_read(&bat_priv->aggregated_ogms) &&
packet_len < BATADV_MAX_AGGREGATION_BYTES)
@@ -709,7 +702,8 @@ static void batadv_iv_ogm_aggregate_new(const unsigned char *packet_buff,
forw_packet_aggr->skb = netdev_alloc_skb_ip_align(NULL, skb_size);
if (!forw_packet_aggr->skb)
- goto out_free_forw_packet;
+ goto packet_free;
+
forw_packet_aggr->skb->priority = TC_PRIO_CONTROL;
skb_reserve(forw_packet_aggr->skb, ETH_HLEN);
@@ -717,12 +711,7 @@ static void batadv_iv_ogm_aggregate_new(const unsigned char *packet_buff,
forw_packet_aggr->packet_len = packet_len;
memcpy(skb_buff, packet_buff, packet_len);
- kref_get(&if_incoming->refcount);
- kref_get(&if_outgoing->refcount);
forw_packet_aggr->own = own_packet;
- forw_packet_aggr->if_incoming = if_incoming;
- forw_packet_aggr->if_outgoing = if_outgoing;
- forw_packet_aggr->num_packets = 0;
forw_packet_aggr->direct_link_flags = BATADV_NO_FLAGS;
forw_packet_aggr->send_time = send_time;
@@ -743,11 +732,8 @@ static void batadv_iv_ogm_aggregate_new(const unsigned char *packet_buff,
send_time - jiffies);
return;
-out_free_forw_packet:
- kfree(forw_packet_aggr);
-out_nomem:
- if (!own_packet)
- atomic_inc(&bat_priv->batman_queue_left);
+packet_free:
+ batadv_forw_packet_free(forw_packet_aggr);
}
/* aggregate a new packet into the existing ogm packet */
@@ -1830,10 +1816,6 @@ static void batadv_iv_send_outstanding_bat_ogm_packet(struct work_struct *work)
batadv_iv_ogm_schedule(forw_packet->if_incoming);
out:
- /* don't count own packet */
- if (!forw_packet->own)
- atomic_inc(&bat_priv->batman_queue_left);
-
batadv_forw_packet_free(forw_packet);
}
@@ -430,6 +430,13 @@ int batadv_send_skb_via_gw(struct batadv_priv *bat_priv, struct sk_buff *skb,
orig_node, vid);
}
+/**
+ * batadv_forw_packet_free - free a forwarding packet
+ * @forw_packet: The packet to free
+ *
+ * This frees a forwarding packet and releases any resources it might
+ * have claimed.
+ */
void batadv_forw_packet_free(struct batadv_forw_packet *forw_packet)
{
kfree_skb(forw_packet->skb);
@@ -437,9 +444,70 @@ void batadv_forw_packet_free(struct batadv_forw_packet *forw_packet)
batadv_hardif_put(forw_packet->if_incoming);
if (forw_packet->if_outgoing)
batadv_hardif_put(forw_packet->if_outgoing);
+ if (forw_packet->queue_left)
+ atomic_inc(forw_packet->queue_left);
kfree(forw_packet);
}
+/**
+ * batadv_forw_packet_alloc - Allocate a forwarding packet
+ * @if_incoming: The (optional) if_incoming to be grabbed
+ * @if_outgoing: The (optional) if_outgoing to be grabbed
+ * @queue_left: The (optional) queue counter to decrease
+ * @bat_priv: The bat_priv for the mesh of this forw_packet
+ *
+ * Allocates a forwarding packet and tries to get a reference to the
+ * (optional) if_incoming, if_outgoing and queue_left. If queue_left
+ * is NULL then bat_priv is optional, too.
+ *
+ * Return: An allocated forwarding packet on success, NULL otherwise.
+ */
+struct batadv_forw_packet *
+batadv_forw_packet_alloc(struct batadv_hard_iface *if_incoming,
+ struct batadv_hard_iface *if_outgoing,
+ atomic_t *queue_left,
+ struct batadv_priv *bat_priv)
+{
+ struct batadv_forw_packet *forw_packet = NULL;
+
+ if (queue_left && !batadv_atomic_dec_not_zero(queue_left)) {
+ if (queue_left == &bat_priv->bcast_queue_left)
+ batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
+ "bcast queue full\n");
+ else if (queue_left == &bat_priv->batman_queue_left)
+ batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
+ "batman queue full\n");
+ else
+ batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
+ "a mysterious queue is full\n");
+ goto out;
+ }
+
+ forw_packet = kmalloc(sizeof(*forw_packet), GFP_ATOMIC);
+ if (!forw_packet)
+ goto err;
+
+ if (if_incoming)
+ kref_get(&if_incoming->refcount);
+
+ if (if_outgoing)
+ kref_get(&if_outgoing->refcount);
+
+ forw_packet->skb = NULL;
+ forw_packet->queue_left = queue_left;
+ forw_packet->if_incoming = if_incoming;
+ forw_packet->if_outgoing = if_outgoing;
+ forw_packet->num_packets = 0;
+
+ goto out;
+
+err:
+ if (queue_left)
+ atomic_inc(queue_left);
+out:
+ return forw_packet;
+}
+
static void
_batadv_add_bcast_packet_to_list(struct batadv_priv *bat_priv,
struct batadv_forw_packet *forw_packet,
@@ -478,20 +546,16 @@ int batadv_add_bcast_packet_to_list(struct batadv_priv *bat_priv,
struct batadv_bcast_packet *bcast_packet;
struct sk_buff *newskb;
- if (!batadv_atomic_dec_not_zero(&bat_priv->bcast_queue_left)) {
- batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
- "bcast packet queue full\n");
- goto out;
- }
-
primary_if = batadv_primary_if_get_selected(bat_priv);
if (!primary_if)
- goto out_and_inc;
-
- forw_packet = kmalloc(sizeof(*forw_packet), GFP_ATOMIC);
+ goto out;
+ forw_packet = batadv_forw_packet_alloc(primary_if, NULL,
+ &bat_priv->bcast_queue_left,
+ bat_priv);
+ batadv_hardif_put(primary_if);
if (!forw_packet)
- goto out_and_inc;
+ goto out;
newskb = skb_copy(skb, GFP_ATOMIC);
if (!newskb)
@@ -504,11 +568,6 @@ int batadv_add_bcast_packet_to_list(struct batadv_priv *bat_priv,
skb_reset_mac_header(newskb);
forw_packet->skb = newskb;
- forw_packet->if_incoming = primary_if;
- forw_packet->if_outgoing = NULL;
-
- /* how often did we send the bcast packet ? */
- forw_packet->num_packets = 0;
INIT_DELAYED_WORK(&forw_packet->delayed_work,
batadv_send_outstanding_bcast_packet);
@@ -517,12 +576,8 @@ int batadv_add_bcast_packet_to_list(struct batadv_priv *bat_priv,
return NETDEV_TX_OK;
packet_free:
- kfree(forw_packet);
-out_and_inc:
- atomic_inc(&bat_priv->bcast_queue_left);
+ batadv_forw_packet_free(forw_packet);
out:
- if (primary_if)
- batadv_hardif_put(primary_if);
return NETDEV_TX_BUSY;
}
@@ -583,7 +638,6 @@ static void batadv_send_outstanding_bcast_packet(struct work_struct *work)
out:
batadv_forw_packet_free(forw_packet);
- atomic_inc(&bat_priv->bcast_queue_left);
}
void
@@ -624,9 +678,6 @@ batadv_purge_outstanding_packets(struct batadv_priv *bat_priv,
if (pending) {
hlist_del(&forw_packet->list);
- if (!forw_packet->own)
- atomic_inc(&bat_priv->bcast_queue_left);
-
batadv_forw_packet_free(forw_packet);
}
}
@@ -654,9 +705,6 @@ batadv_purge_outstanding_packets(struct batadv_priv *bat_priv,
if (pending) {
hlist_del(&forw_packet->list);
- if (!forw_packet->own)
- atomic_inc(&bat_priv->batman_queue_left);
-
batadv_forw_packet_free(forw_packet);
}
}
@@ -28,6 +28,12 @@
struct sk_buff;
void batadv_forw_packet_free(struct batadv_forw_packet *forw_packet);
+struct batadv_forw_packet *
+batadv_forw_packet_alloc(struct batadv_hard_iface *if_incoming,
+ struct batadv_hard_iface *if_outgoing,
+ atomic_t *queue_left,
+ struct batadv_priv *bat_priv);
+
int batadv_send_skb_to_orig(struct sk_buff *skb,
struct batadv_orig_node *orig_node,
struct batadv_hard_iface *recv_if);
@@ -1375,6 +1375,7 @@ struct batadv_skb_cb {
* locally generated packet
* @if_outgoing: packet where the packet should be sent to, or NULL if
* unspecified
+ * @queue_left: The queue (counter) this packet was applied to
*/
struct batadv_forw_packet {
struct hlist_node list;
@@ -1387,6 +1388,7 @@ struct batadv_forw_packet {
struct delayed_work delayed_work;
struct batadv_hard_iface *if_incoming;
struct batadv_hard_iface *if_outgoing;
+ atomic_t *queue_left;
};
/**