Skip to contentSkip to conversation

atmodem/atutil.h: cb_data ref/unref possibility

PATCH Superseded Giacinto Cifelli 3 participants 4 messages Oct 25, 2018

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 atmodem/atutil.h: cb_data ref/unref possibility

the cb_data can be used by creating the structure with cb_data_new, and then there are two possibilities:
- use it in a single callback function, and destroy it with a call to g_free.
Example:
- calling function:

    struct cb_data *cbd = cb_data_new(cb, data);
    if (g_at_chat_send(chat, buf, NULL, at_cgatt_cb, cbd, g_free) > 0)
	return;
    g_free(cbd);
  - called function (here at_cgatt_cb):
	static void at_cgatt_cb(gboolean ok, GAtResult *result,
						gpointer user_data)
	{
		struct cb_data *cbd = user_data;
		ofono_gprs_cb_t cb = cbd->cb;
		struct ofono_error error;

		decode_at_error(&error,
				g_at_result_final_response(result));

		cb(&error, cbd->data);
	}
    note the absence of explicit g_free(cbd);

- pass it through a train of callback functions, adding a reference at each pass cb_data_ref, and removing it with cb_data_unref. the use of cb_data_ref would replace a new object creation, while the use of cb_data_unref the use of g_free.
Example:
- calling function:

	struct cb_data *cbd = cb_data_new(cb, data);
	// no cb_ref at the creation
	if (g_at_chat_send(chat, buf, NULL,
				at_lte_set_default_attach_info_cb,
				cbd, cb_data_unref) > 0)
		goto end;
	cb_data_unref(cbd);
  - called function 1 (at_lte_set_default_attach_info_cb):
	static void at_lte_set_default_attach_info_cb(gboolean ok,
				GAtResult *result, gpointer user_data)
	{
		struct cb_data *cbd = user_data;

		cbd = cb_data_ref(cbd);
		if (g_at_chat_send(chat, buf, NULL,
				at_cgatt_cb, cbd, cb_data_unref) > 0)
			return;
		cb_data_unref(cbd);
	}
  - called function 2 (at_cgatt_cb):
    like above. no call to g_free or cb_data_unref. The terminal function
    doesn't need to know about the reference scheme.
---
 drivers/atmodem/atutil.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
drivers/atmodem/atutil.h +24 -0
index f1389a94..190ee17f 100644
@@ -114,12 +114,36 @@ static inline struct cb_data *cb_data_new(void *cb, void *data)
 	struct cb_data *ret;
 
 	ret = g_new0(struct cb_data, 1);
+	ret->ref_count = 1;
 	ret->cb = cb;
 	ret->data = data;
 
 	return ret;
 }
 
+static inline struct cb_data *cb_data_ref(struct cb_data *cbd)
+{
+	if (cbd == NULL)
+		return NULL;
+
+	g_atomic_int_inc(&cbd->ref_count);
+
+	return cbd;
+}
+
+static inline void cb_data_unref(struct cb_data *cbd)
+{
+	gboolean is_zero;
+
+	if (cbd == NULL)
+		return;
+
+	is_zero = g_atomic_int_dec_and_test(&cbd->ref_count);
+
+	if (is_zero == TRUE)
+		g_free(cbd);
+}
+
 static inline int at_util_convert_signal_strength(int strength)
 {
 	int result;
Signature · 1 line
2.17.1
#1 lore raw

Hi Giacinto,

drivers/atmodem/atutil.h static inline struct cb_data *cb_data_new(void *cb, void *data) · line 130

Quoted 7 lines · Giacinto Cifelli
drivers/atmodem/atutil.h +7 -0
+static inline struct cb_data *cb_data_ref(struct cb_data *cbd)
+{
+	if (cbd == NULL)
+		return NULL;
+
+	g_atomic_int_inc(&cbd->ref_count);
+

Let me just vent my disagreement with using atomic accessors here again:

i) If I'm a new contributor, unfamiliar with ofono, and I see this then I think: "oh oh, this is a thread-safe library... better be careful."... and then I get all confused because nothing else is handled in a thread-safe manner.

ii) The atomic operations introduce an unnecessary memory barrier.

I _presume_ that the code is written this way because it was inherited from connman and connman made an early attempt to be thread safe...??? I'm not sure that legacy is worth preserving.

Anyway, enough said on the matter. The implementation is fine aside from my reservations.

/Jonas

Quoted 20 lines
drivers/atmodem/atutil.h +16 -0
+	return cbd;
+}
+
+static inline void cb_data_unref(struct cb_data *cbd)
+{
+	gboolean is_zero;
+
+	if (cbd == NULL)
+		return;
+
+	is_zero = g_atomic_int_dec_and_test(&cbd->ref_count);
+
+	if (is_zero == TRUE)
+		g_free(cbd);
+}
+
  static inline int at_util_convert_signal_strength(int strength)
  {
  	int result;
#2 lore raw

Hi Jonas,

Quoted 20 lines · Jonas Bonn

Hi Giacinto,

drivers/atmodem/atutil.h static inline struct cb_data *cb_data_new(void *cb, void *data) · line 130

Quoted 7 lines · Giacinto Cifelli
drivers/atmodem/atutil.h +7 -0
+static inline struct cb_data *cb_data_ref(struct cb_data *cbd)
+{
+     if (cbd == NULL)
+             return NULL;
+
+     g_atomic_int_inc(&cbd->ref_count);
+

Let me just vent my disagreement with using atomic accessors here again:

i) If I'm a new contributor, unfamiliar with ofono, and I see this then I think: "oh oh, this is a thread-safe library... better be careful."... and then I get all confused because nothing else is handled in a thread-safe manner.

Quoted 8 lines

ii) The atomic operations introduce an unnecessary memory barrier.

I _presume_ that the code is written this way because it was inherited from connman and connman made an early attempt to be thread safe...??? I'm not sure that legacy is worth preserving.

Anyway, enough said on the matter. The implementation is fine aside from my reservations.

overall I agree with you, but I have no feelings for the code. If not needed otherwise, I adapt to the various projects I contribute, in style and preferences.
Here was decided by Marcel that this is the only way, and I adhere to this.

But, since I am investing quite some time on ofono, and it is not a company internal project,
I plan to write a book on it, where I could also mention that it is not thread-safe, despite the occasional presence of atomic operations.

Quoted 3 lines

/Jonas

Giacinto

#3 lore raw
In reply to Jonas Bonn

Hi Jonas,

Quoted 19 lines · Jonas Bonn

Hi Giacinto,

drivers/atmodem/atutil.h static inline struct cb_data *cb_data_new(void *cb, void *data) · line 130

Quoted 7 lines · Giacinto Cifelli
drivers/atmodem/atutil.h +7 -0
+static inline struct cb_data *cb_data_ref(struct cb_data *cbd)
+{
+    if (cbd == NULL)
+        return NULL;
+
+    g_atomic_int_inc(&cbd->ref_count);
+

Let me just vent my disagreement with using atomic accessors here again:

i)  If I'm a new contributor, unfamiliar with ofono, and I see this then I think:  "oh oh, this is a thread-safe library... better be careful."... and then I get all confused because nothing else is handled in a thread-safe manner.

Quoted 8 lines

ii)  The atomic operations introduce an unnecessary memory barrier.

I _presume_ that the code is written this way because it was inherited from connman and connman made an early attempt to be thread safe...??? I'm not sure that legacy is worth preserving.

Anyway, enough said on the matter.  The implementation is fine aside from my reservations.

So let me make you happy:

For all new code the policy is that g_atomic should not be used. Once we switch to ell for oFono 2.x (or even before that) we should have no instances of g_atomic in the codebase.

If someone wants to start removing some of these now, be my guest.

Regards,
-Denis