Hi, Luiz On Fri, Sep 29, 2017 at 1:02 AM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: > Hi Yunhan, > > On Fri, Sep 29, 2017 at 10:50 AM, Yunhan Wang <yunhanw@xxxxxxxxxxxx> wrote: >> Hi, Luiz >> >> I see. Great! Thanks for your explanation. > > Please comment inline on the list. > >> Maybe it is good to have both solutions in current bluez? >> 1. AcquireWrite, write data passing via fd, mtu passing by DBUS, we >> can take advantage of fd under high load. >> 2. Write, write data and mtu passing over DBUS, we still can fully use >> DBUS under low load. > > Well that is actually a 3 type of transfer since regular WriteValue > already exists which defeat a little bit the purpose of WriteAcquired > since we can no longer determine if the server supports fd mode or > not, I think it is too much and the server has to either to decide if > unfragmented D-Bus transfer is fine is not, if not then fd passing > shall be the only mechanism. > I agree, it is three type of transfer, regular write, regular write with mtu support(application is responsible for packet fragmentation) fd with pipe (application is responsible for packet fragmentation), now we have two kind of transfer(regular write, acquire write) in code, add third one is pretty straightforward and only small code change. I read two articles(https://blogs.gnome.org/abustany/2010/05/20/ipc-performance-the-return-of-the-report/, https://lists.freedesktop.org/archives/dbus/2014-October/016363.html), I agree that fd with pipe is more efficent, and eliminate several copy, and benefit for byte streaming although there is minor vulnerability mentioned in above article, I hope engineer in peripheral application can still have another choice, regular write with mtu, when traffic is low or it is experiment purpose. Considering these are pretty similar, easy to integrate, engineer still can easily switch them between fd with pipe and regular write. > Considering how easy it was to add support for fd passing on the > bluetoothctl I don't thing it is that big of deal really, or there is > something else preventing this to happen in your end? > Yes, fd passing is pretty easy and efficent, it is not big deal to integrate it, using this way, in my end, for gatt write with response from central to peripheral, I can see written value and length in https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n688, but I cannot see any response back for GATT write from peripheral to central. It seems this only support GATT write without response even though I have removed the check here(https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n1360). Any idea? In contrast, for regular WriteValue, I can see write response from peripheral to central. >> which means the only light change is to add mtu passing over DBUS in 2. Write >> >> Finally both solutions can be available to bluez users. >> >> Thanks >> Best wishes >> Yunhan >> >> On Fri, Sep 29, 2017 at 12:22 AM, Luiz Augusto von Dentz >> <luiz.dentz@xxxxxxxxx> wrote: >>> Hi Yunhan, >>> >>> On Fri, Sep 29, 2017 at 10:14 AM, Yunhan Wang <yunhanw@xxxxxxxxxxxx> wrote: >>>> Hi, Luiz >>>> >>>> For server, what is the benefit to use fd instead of DBUS to pass >>>> write value? Isn't it better that we put all bluez client and server >>>> only chat via DBUS upon bluetoothd? For mtu, definitely we can do it >>>> using DBUS message, if we use current acquire_write, in server >>>> application side, we will have to implement callback for >>>> "AcquireWrite" instead of "WriteValue“、 >>> >>> For byte streams passing data over D-Bus is inefficient as that has is >>> delivered to dbus-daemon before the end application it will always be >>> slower than sending directly via fd, actually depending on the load on >>> the system this may impact other processes using D-Bus as the daemon >>> will be busy processing the WriteValue messages. >>> >>>> Thanks >>>> Best wishes >>>> Yunhan >>>> >>>> On Thu, Sep 28, 2017 at 11:54 PM, Luiz Augusto von Dentz >>>> <luiz.dentz@xxxxxxxxx> wrote: >>>>> Hi Yunhan, >>>>> >>>>> On Fri, Sep 29, 2017 at 8:04 AM, Yunhan Wang <yunhanw@xxxxxxxxxxxx> wrote: >>>>>> Hi, Luiz >>>>>> >>>>>> May I ask several questions about acquire write implementation? >>>>>> >>>>>> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz >>>>>> <luiz.dentz@xxxxxxxxx> wrote: >>>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx> >>>>>>> >>>>>>> This enables IO via file descriptors using AcquireWrite if server >>>>>>> implements it. >>>>>>> --- >>>>>>> src/gatt-database.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 142 insertions(+) >>>>>>> >>>>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>>>>>> index 61eed71d6..239fe5384 100644 >>>>>>> --- a/src/gatt-database.c >>>>>>> +++ b/src/gatt-database.c >>>>>>> @@ -33,6 +33,7 @@ >>>>>>> #include "gdbus/gdbus.h" >>>>>>> #include "src/shared/util.h" >>>>>>> #include "src/shared/queue.h" >>>>>>> +#include "src/shared/io.h" >>>>>>> #include "src/shared/att.h" >>>>>>> #include "src/shared/gatt-db.h" >>>>>>> #include "src/shared/gatt-server.h" >>>>>>> @@ -117,6 +118,7 @@ struct external_chrc { >>>>>>> uint8_t props; >>>>>>> uint8_t ext_props; >>>>>>> uint32_t perm; >>>>>>> + struct io *write_io; >>>>>>> struct gatt_db_attribute *attrib; >>>>>>> struct gatt_db_attribute *ccc; >>>>>>> struct queue *pending_reads; >>>>>>> @@ -325,6 +327,8 @@ static void chrc_free(void *data) >>>>>>> { >>>>>>> struct external_chrc *chrc = data; >>>>>>> >>>>>>> + io_destroy(chrc->write_io); >>>>>>> + >>>>>>> queue_destroy(chrc->pending_reads, cancel_pending_read); >>>>>>> queue_destroy(chrc->pending_writes, cancel_pending_write); >>>>>>> >>>>>>> @@ -1789,6 +1793,128 @@ static struct pending_op *send_write(struct btd_device *device, >>>>>>> return NULL; >>>>>>> } >>>>>>> >>>>>>> +static bool pipe_hup(struct io *io, void *user_data) >>>>>>> +{ >>>>>>> + struct external_chrc *chrc = user_data; >>>>>>> + >>>>>>> + DBG("%p closed\n", io); >>>>>>> + >>>>>>> + if (io == chrc->write_io) >>>>>>> + chrc->write_io = NULL; >>>>>>> + >>>>>>> + io_destroy(io); >>>>>>> + >>>>>>> + return false; >>>>>>> +} >>>>>>> + >>>>>>> +static struct io *pipe_io_new(int fd, void *user_data) >>>>>>> +{ >>>>>>> + struct io *io; >>>>>>> + >>>>>>> + io = io_new(fd); >>>>>>> + >>>>>>> + io_set_close_on_destroy(io, true); >>>>>>> + >>>>>>> + io_set_disconnect_handler(io, pipe_hup, user_data, NULL); >>>>>>> + >>>>>>> + return io; >>>>>>> +} >>>>>>> + >>>>>>> +static int pipe_io_send(struct io *io, const void *data, size_t len) >>>>>>> +{ >>>>>>> + struct iovec iov; >>>>>>> + >>>>>>> + iov.iov_base = (void *) data; >>>>>>> + iov.iov_len = len; >>>>>>> + >>>>>>> + return io_send(io, &iov, 1); >>>>>>> +} >>>>>>> + >>>>>>> +static void acquire_write_reply(DBusMessage *message, void *user_data) >>>>>>> +{ >>>>>>> + struct pending_op *op = user_data; >>>>>>> + struct external_chrc *chrc; >>>>>>> + DBusError err; >>>>>>> + int fd; >>>>>>> + uint16_t mtu; >>>>>>> + >>>>>>> + chrc = gatt_db_attribute_get_user_data(op->attrib); >>>>>>> + dbus_error_init(&err); >>>>>>> + >>>>>>> + if (dbus_set_error_from_message(&err, message) == TRUE) { >>>>>>> + error("Failed to acquire write: %s\n", err.name); >>>>>>> + dbus_error_free(&err); >>>>>>> + goto retry; >>>>>>> + } >>>>>>> + >>>>>>> + if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &fd, >>>>>>> + DBUS_TYPE_UINT16, &mtu, >>>>>>> + DBUS_TYPE_INVALID) == false)) { >>>>>>> + error("Invalid AcquireWrite response\n"); >>>>>>> + goto retry; >>>>>>> + } >>>>>>> + >>>>>>> + DBG("AcquireWrite success: fd %d MTU %u\n", fd, mtu); >>>>>>> + >>>>>>> + chrc->write_io = pipe_io_new(fd, chrc); >>>>>>> + >>>>>>> + if (pipe_io_send(chrc->write_io, op->data.iov_base, >>>>>>> + op->data.iov_len) < 0) >>>>>>> + goto retry; >>>>>>> + >>>>>>> + return; >>>>>>> + >>>>>>> +retry: >>>>>>> + send_write(op->device, op->attrib, chrc->proxy, NULL, op->id, >>>>>>> + op->data.iov_base, op->data.iov_len, 0); >>>>>>> +} >>>>>>> + >>>>>>> +static void acquire_write_setup(DBusMessageIter *iter, void *user_data) >>>>>>> +{ >>>>>>> + struct pending_op *op = user_data; >>>>>>> + DBusMessageIter dict; >>>>>>> + struct bt_gatt_server *server; >>>>>>> + uint16_t mtu; >>>>>>> + >>>>>>> + dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, >>>>>>> + DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING >>>>>>> + DBUS_TYPE_STRING_AS_STRING >>>>>>> + DBUS_TYPE_VARIANT_AS_STRING >>>>>>> + DBUS_DICT_ENTRY_END_CHAR_AS_STRING, >>>>>>> + &dict); >>>>>>> + >>>>>>> + append_options(&dict, op); >>>>>>> + >>>>>>> + server = btd_device_get_gatt_server(op->device); >>>>>>> + >>>>>>> + mtu = bt_gatt_server_get_mtu(server); >>>>>>> + >>>>>>> + dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &mtu); >>>>>>> + >>>>>>> + dbus_message_iter_close_container(iter, &dict); >>>>>>> +} >>>>>>> + >>>>>>> +static struct pending_op *acquire_write(struct external_chrc *chrc, >>>>>>> + struct btd_device *device, >>>>>>> + struct gatt_db_attribute *attrib, >>>>>>> + unsigned int id, >>>>>>> + const uint8_t *value, size_t len) >>>>>>> +{ >>>>>>> + struct pending_op *op; >>>>>>> + >>>>>>> + op = pending_write_new(device, NULL, attrib, id, value, len, 0); >>>>>>> + >>>>>>> + if (g_dbus_proxy_method_call(chrc->proxy, "AcquireWrite", >>>>>>> + acquire_write_setup, >>>>>>> + acquire_write_reply, >>>>>>> + op, pending_op_free)) >>>>>>> + return op; >>>>>>> + >>>>>>> + pending_op_free(op); >>>>>>> + >>>>>>> + return NULL; >>>>>>> +} >>>>>>> + >>>>>>> static uint8_t ccc_write_cb(uint16_t value, void *user_data) >>>>>>> { >>>>>>> struct external_chrc *chrc = user_data; >>>>>>> @@ -2090,6 +2216,7 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib, >>>>>>> struct external_chrc *chrc = user_data; >>>>>>> struct btd_device *device; >>>>>>> struct queue *queue; >>>>>>> + DBusMessageIter iter; >>>>>>> >>>>>>> if (chrc->attrib != attrib) { >>>>>>> error("Write callback called with incorrect attribute"); >>>>>>> @@ -2102,6 +2229,21 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib, >>>>>>> goto fail; >>>>>>> } >>>>>>> >>>>>>> + if (chrc->write_io) { >>>>>>> + if (pipe_io_send(chrc->write_io, value, len) < 0) { >>>>>>> + error("Unable to write: %s", strerror(errno)); >>>>>>> + goto fail; >>>>>>> + } >>>>>>> + >>>>>>> + gatt_db_attribute_write_result(attrib, id, 0); >>>>>>> + return; >>>>>>> + } >>>>>>> + >>>>>> Could you elaborate more about this part of codes, why are you using >>>>>> pipe_io_send to update value instead of dbus? >>>>> >>>>> This is doing the write via fd if write_io has been acquired, the >>>>> whole point is avoiding D-Bus. >>>>> >>>>>>> + if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &iter)) { >>>>>>> + if (acquire_write(chrc, device, attrib, id, value, len)) >>>>>>> + return; >>>>>>> + } >>>>>>> + >>>>>> >>>>>> Currently my expected scenario is to get MTU when first Gatt Write( >>>>>> write with response) comes in specific characteristic, for example, I >>>>>> use iphone as ble central and write value in Characteristic A on Bluez >>>>>> peripheral, Bluez peripheral get negotiated MTU when first write comes >>>>>> in. From this part of codes, it seems it return immediately after >>>>>> acquire_write is called, how can we move forward and write value to >>>>>> peripheral application? What is the design idea here? >>>>> >>>>> After AcquireWrite the data is sent via fd as well: >>>>> >>>>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-database.c#n1891 >>>>> >>>>>> The below is earlier draft idea for this MTU acquisition in server side. >>>>>> https://www.spinics.net/lists/linux-bluetooth/msg71961.html >>>>>> >>>>>>> if (!(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP)) >>>>>>> queue = chrc->pending_writes; >>>>>>> else >>>>>>> -- >>>>>>> 2.13.5 >>>>>>> >>>>>>> -- >>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >>>>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> >>>>> >>>>> >>>>> -- >>>>> Luiz Augusto von Dentz >>> >>> >>> >>> -- >>> Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
